Re: [PATCH v14 5/5] arm/vpci: honor access size when returning an error

[Julien Grall <julien@xxxxxxx>]

Hi Stewart,

On 14/05/2024 15:33, Stewart Hildebrand wrote:
> From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
With one remark below:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> 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