| Message ID | 1421684957-29884-6-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ian, On 20/01/15 16:00, Ian Campbell wrote: > On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote: >> The current vGIC v3 driver doesn't fully implement GICv3 spec: >> - GICv3 backward compatibility is not supported (GICD_CTLR.ARE = 0) > > I think you meant GICv2 here as you did in the code. Yes. > In which case I believe this is optional in the spec, i.e. we can be > compliant and still not implement this. > > That's not to say it isn't desirable, but this is a TODO item, not a > spec non-conformity issue. Right, I should rename the section to TODO. >> - A processor can only access his own redistributor. For buggy >> assumption, the current code bank the redistributors MMIO. > > What assumption? It's not clear if you mean that a foreign redistributor > should not be accessible and is, or if it should be accessible and > isn't. Every redistributor (one per processor) are mapped in distinct MMIO region. Unlike the distributor, the redistributor is not banked. Our current implementation (see vgic_v3_rdistr_mmio_write) consider that the redistributor is banked and replicate n-times in the memory. If you give a look to the redistributor iniatialization (see Xen and Linux GICv3 code). The code will go through all the redistributors and check GICR_TYPER to see if the processor is associated to this redistributor. I'm not sure how the redistributor should behave if it's accessed by another processor. But I'm sure it's wrong to bank it. Regards,
On 21/01/15 12:16, Ian Campbell wrote: > On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote: > >>>> - A processor can only access his own redistributor. For buggy >>>> assumption, the current code bank the redistributors MMIO. >>> >>> What assumption? It's not clear if you mean that a foreign redistributor >>> should not be accessible and is, or if it should be accessible and >>> isn't. >> >> Every redistributor (one per processor) are mapped in distinct MMIO region. >> >> Unlike the distributor, the redistributor is not banked. > > Understood. > >> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that >> the redistributor is banked and replicate n-times in the memory. > > IOW instead of having e.g. 8 individual redistributors each vcpu sees > it's own redistributor 8 times. That does seem a bit dubious. It's the current behavior. You can see the difference in linux log. The address of each redistributor is the same. >> If you give a look to the redistributor iniatialization (see Xen and >> Linux GICv3 code). The code will go through all the redistributors and >> check GICR_TYPER to see if the processor is associated to this >> redistributor. >> >> I'm not sure how the redistributor should behave if it's accessed by >> another processor. > > Please can you find a spec reference and include it in the clarified > version of this item. Rather than the distributor, there is multiple redistributor (one per processor). I think the section 5.4.1 in the GICv3 should answer to this question: "Each re-distributor must be allocated at one page for controlling the overall behavior of the re-distributor and for controlling physical LPIs. The base address of this page is referred to as RD_base. In addition, each re-distributor must be also allocated the following additional pages". Regards,
On 21/01/15 12:48, Ian Campbell wrote: > On Wed, 2015-01-21 at 12:33 +0000, Julien Grall wrote: >> On 21/01/15 12:16, Ian Campbell wrote: >>> On Tue, 2015-01-20 at 17:49 +0000, Julien Grall wrote: >>> >>>>>> - A processor can only access his own redistributor. For buggy >>>>>> assumption, the current code bank the redistributors MMIO. >>>>> >>>>> What assumption? It's not clear if you mean that a foreign redistributor >>>>> should not be accessible and is, or if it should be accessible and >>>>> isn't. >>>> >>>> Every redistributor (one per processor) are mapped in distinct MMIO region. >>>> >>>> Unlike the distributor, the redistributor is not banked. >>> >>> Understood. >>> >>>> Our current implementation (see vgic_v3_rdistr_mmio_write) consider that >>>> the redistributor is banked and replicate n-times in the memory. >>> >>> IOW instead of having e.g. 8 individual redistributors each vcpu sees >>> it's own redistributor 8 times. That does seem a bit dubious. >> >> It's the current behavior. You can see the difference in linux log. The >> address of each redistributor is the same. > > Are you sure that isn't the "redistributor region"? One of those can > contain multiple redistributors, i.e. gic_v3_init sets up a single > region and that propagates to the DTB given to the guest. > > So the error is just in the mmio decode stage I think, not in the setup > we are trying to achieve. Right, the device tree is valid, we expose a region containing enough space for up to 8 distributor. The problem is in our MMIO emulation. >>>> If you give a look to the redistributor iniatialization (see Xen and >>>> Linux GICv3 code). The code will go through all the redistributors and >>>> check GICR_TYPER to see if the processor is associated to this >>>> redistributor. >>>> >>>> I'm not sure how the redistributor should behave if it's accessed by >>>> another processor. >>> >>> Please can you find a spec reference and include it in the clarified >>> version of this item. >> >> Rather than the distributor, there is multiple redistributor (one per >> processor). >> >> I think the section 5.4.1 in the GICv3 should answer to this question: >> >> "Each re-distributor must be allocated at one page for controlling the >> overall behavior of the re-distributor and for controlling physical >> LPIs. The base address of this page is referred to as RD_base. In >> addition, each re-distributor must be also allocated the following >> additional pages". > > That doesn't say anything about one CPU touching another's > redistributor. Linux is at least using GICR_TYPER, to retrieve the right distributor. For the other the register I've no idea. I've asked ARM. Let see what they will answer.
Hi, On 21/01/15 13:19, Julien Grall wrote: >>>>> If you give a look to the redistributor iniatialization (see Xen and >>>>> Linux GICv3 code). The code will go through all the redistributors and >>>>> check GICR_TYPER to see if the processor is associated to this >>>>> redistributor. >>>>> >>>>> I'm not sure how the redistributor should behave if it's accessed by >>>>> another processor. >>>> >>>> Please can you find a spec reference and include it in the clarified >>>> version of this item. >>> >>> Rather than the distributor, there is multiple redistributor (one per >>> processor). >>> >>> I think the section 5.4.1 in the GICv3 should answer to this question: >>> >>> "Each re-distributor must be allocated at one page for controlling the >>> overall behavior of the re-distributor and for controlling physical >>> LPIs. The base address of this page is referred to as RD_base. In >>> addition, each re-distributor must be also allocated the following >>> additional pages". >> >> That doesn't say anything about one CPU touching another's >> redistributor. > > Linux is at least using GICR_TYPER, to retrieve the right distributor. > For the other the register I've no idea. I've asked ARM. Let see what > they will answer. For documentation purpose, the re-distributor should be accessible by any processor. It has been confirmed by ARM and I've found the paragraph in spec stating: "GICR_* registers. Multiple pages of registers per re-distributor accessible to all processors." Section 4.3.5 of the r24 document. I have a working implementation of this behavior. I will add it in the v2 of this series and drop this patch. Regards,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 1fa1413..9818a6b 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -16,6 +16,11 @@ * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. + * + * Current limitation of the vGIC v3: + * - GICv2 backward compatibility is not supported (GICD_CTRL.ARE = 0) + * - A processor can only access his own redistributor. For buggy + * assumption, the current code bank the redistributors MMIO */ #include <xen/bitops.h>
The current vGIC v3 driver doesn't fully implement GICv3 spec: - GICv3 backward compatibility is not supported (GICD_CTLR.ARE = 0) - A processor can only access his own redistributor. For buggy assumption, the current code bank the redistributors MMIO. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/vgic-v3.c | 5 +++++ 1 file changed, 5 insertions(+)