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>
---
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;
         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;
 }
-- 
2.43.2

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,

-- 
Julien Grall

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.