| Message ID | 20240514143400.152280-6-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
Hi Stewart, On 14/05/2024 15:33, Stewart Hildebrand wrote: > From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > Guest can try to read config space using different access sizes: 8, > 16, 32, 64 bits. We need to take this into account when we are > returning an error back to MMIO handler, otherwise it is possible to > provide more data than requested: i.e. guest issues LDRB instruction > to read one byte, but we are writing 0xFFFFFFFFFFFFFFFF in the target > register. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> With one remark below: Acked-by: Julien Grall <jgrall@amazon.com> > --- > v9->10: > * New patch in v10. > --- > xen/arch/arm/vpci.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 348ba0fbc860..aaf9d9120c3d 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -41,6 +41,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > { > struct pci_host_bridge *bridge = p; > pci_sbdf_t sbdf; > + const uint8_t access_size = (1 << info->dabt.size) * 8; > + const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0); > /* data is needed to prevent a pointer cast on 32bit */ > unsigned long data; > > @@ -48,7 +50,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > > if ( !vpci_sbdf_from_gpa(v->domain, bridge, info->gpa, &sbdf) ) > { > - *r = ~0UL; > + *r = access_mask; The name 'access_mask' is a bit confusing. I would not expect such value for be returned to the guest. What about 'invalid'? Also can you confirm whether patches #4 and #5 be committed without the rest of the series? Cheers,
On 5/14/24 13:48, Julien Grall wrote: > Hi Stewart, > > On 14/05/2024 15:33, Stewart Hildebrand wrote: >> From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> >> Guest can try to read config space using different access sizes: 8, >> 16, 32, 64 bits. We need to take this into account when we are >> returning an error back to MMIO handler, otherwise it is possible to >> provide more data than requested: i.e. guest issues LDRB instruction >> to read one byte, but we are writing 0xFFFFFFFFFFFFFFFF in the target >> register. >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > > With one remark below: > > Acked-by: Julien Grall <jgrall@amazon.com> Thanks! > >> --- >> v9->10: >> * New patch in v10. >> --- >> xen/arch/arm/vpci.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> index 348ba0fbc860..aaf9d9120c3d 100644 >> --- a/xen/arch/arm/vpci.c >> +++ b/xen/arch/arm/vpci.c >> @@ -41,6 +41,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> { >> struct pci_host_bridge *bridge = p; >> pci_sbdf_t sbdf; >> + const uint8_t access_size = (1 << info->dabt.size) * 8; I'd like to add a U suffix to the 1 to make it consistent with the remaining occurrences in this file. >> + const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0); >> /* data is needed to prevent a pointer cast on 32bit */ >> unsigned long data; >> @@ -48,7 +50,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> if ( !vpci_sbdf_from_gpa(v->domain, bridge, info->gpa, &sbdf) ) >> { >> - *r = ~0UL; >> + *r = access_mask; > > The name 'access_mask' is a bit confusing. I would not expect such value > for be returned to the guest. What about 'invalid'? That sounds good, I've made the change in my local tree. > > Also can you confirm whether patches #4 and #5 be committed without > the rest of the series? Patch #4: no, it uses a constant defined in patch #2 ("vpci: add initial support for virtual PCI bus topology"). Patch #5: conceptually, yes, but patch #3 ("xen/arm: translate virtual PCI bus topology for guests") also modifies vpci_mmio_read(), so there are rebase conflicts to resolve in both patches #3 and #5. Thinking more about it, patch #5 falls more into the category of a fix than a feature, so it probably should have been in the beginning of the series anyway. Alright, I've reordered it and resolved the rebase conflicts in my local tree. Here's what the patch ("arm/vpci: honor access size when returning an error") now looks like based on staging: diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 3bc4bb55082a..31e9e1d20751 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -29,6 +29,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, { struct pci_host_bridge *bridge = p; pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); + const uint8_t access_size = (1U << info->dabt.size) * 8; + const uint64_t invalid = GENMASK_ULL(access_size - 1, 0); /* data is needed to prevent a pointer cast on 32bit */ unsigned long data; @@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, return 1; } - *r = ~0ul; + *r = invalid; return 0; } The patch ("xen/arm: translate virtual PCI bus topology for guests") will then introduce a new use of the "invalid" variable.
On 14.05.2024 22:31, Stewart Hildebrand wrote: > Here's what the patch ("arm/vpci: honor access size when returning an > error") now looks like based on staging: > > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 3bc4bb55082a..31e9e1d20751 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -29,6 +29,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > { > struct pci_host_bridge *bridge = p; > pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + const uint8_t access_size = (1U << info->dabt.size) * 8; And why exactly uint8_t here, rather than unsigned int? See ./CODING_STYLE. > + const uint64_t invalid = GENMASK_ULL(access_size - 1, 0); I'm not entirely convinced of uint64_t here either, but I'd view this as more borderline than the uint8_t above. As per ... > @@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > return 1; > } > > - *r = ~0ul; > + *r = invalid; ... the original rhs here, unsigned long (or perhaps register_t) would seem more appropriate, but I have no idea whether on Arm32 info->dabt.size can end up being 3. Jan
On 5/15/24 02:32, Jan Beulich wrote: > On 14.05.2024 22:31, Stewart Hildebrand wrote: >> Here's what the patch ("arm/vpci: honor access size when returning an >> error") now looks like based on staging: >> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> index 3bc4bb55082a..31e9e1d20751 100644 >> --- a/xen/arch/arm/vpci.c >> +++ b/xen/arch/arm/vpci.c >> @@ -29,6 +29,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> { >> struct pci_host_bridge *bridge = p; >> pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >> + const uint8_t access_size = (1U << info->dabt.size) * 8; > > And why exactly uint8_t here, rather than unsigned int? See ./CODING_STYLE. I'll change to unsigned int. > >> + const uint64_t invalid = GENMASK_ULL(access_size - 1, 0); > > I'm not entirely convinced of uint64_t here either, but I'd view this as > more borderline than the uint8_t above. As per ... > >> @@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> return 1; >> } >> >> - *r = ~0ul; >> + *r = invalid; > > ... the original rhs here, unsigned long (or perhaps register_t) would seem > more appropriate, but I have no idea whether on Arm32 info->dabt.size can > end up being 3. Well, it depends which spec we look at. In the ARMv7 ARM, 3 is reserved. But in the ARMv8 ARM, in the aarch32 section, 3 appears to be valid... Anyway, since the value returned in r can only be as wide as register_t due to the way our mmio handlers are implemented, I'll change to register_t for now to match the lhs.
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 348ba0fbc860..aaf9d9120c3d 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -41,6 +41,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, { struct pci_host_bridge *bridge = p; pci_sbdf_t sbdf; + const uint8_t access_size = (1 << info->dabt.size) * 8; + const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0); /* data is needed to prevent a pointer cast on 32bit */ unsigned long data; @@ -48,7 +50,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, if ( !vpci_sbdf_from_gpa(v->domain, bridge, info->gpa, &sbdf) ) { - *r = ~0UL; + *r = access_mask; return 1; } @@ -59,7 +61,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, return 1; } - *r = ~0UL; + *r = access_mask; return 0; }