On 5/14/24 13:48, Julien Grall wrote:
> Hi Stewart,
>
> On 14/05/2024 15:33, Stewart Hildebrand wrote:
>> From: Volodymyr Babchuk <volodymyr_babc...@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_babc...@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
>
> With one remark below:
>
> Acked-by: Julien Grall <jgr...@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.
