[0/2] s390x/pci: some pcistb fixes

Message

Matthew Rosato Dec. 17, 2020, 2:58 p.m. UTC

Here are a few fixes pulled out of the 'Fixing s390 vfio-pci ISM support'
patchset.

Matthew Rosato (2):
  s390x/pci: fix pcistb length
  s390x/pci: Fix memory_region_access_valid call

 hw/s390x/s390-pci-inst.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Thomas Huth Dec. 17, 2020, 3:12 p.m. UTC | #1

On 17/12/2020 15.58, Matthew Rosato wrote:
> In pcistb_service_handler, a call is made to validate that the memory
> region can be accessed.  However, the call is made using the entire length
> of the pcistb operation, which can be larger than the allowed memory
> access size (8).  Since we already know that the provided buffer is a
> multiple of 8, fix the call to memory_region_access_valid to iterate
> over the memory region in the same way as the subsequent call to
> memory_region_dispatch_write.
> 
> Fixes: 863f6f52b7 ("s390: implement pci instructions")
> Signed-off-by: Matthew Rosato <[e-mail]>
> ---
>  hw/s390x/s390-pci-inst.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index e230293..f93f8ba 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -821,10 +821,12 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      mr = s390_get_subregion(mr, offset, len);
>      offset -= mr->addr;
>  
> -    if (!memory_region_access_valid(mr, offset, len, true,
> -                                    MEMTXATTRS_UNSPECIFIED)) {
> -        s390_program_interrupt(env, PGM_OPERAND, ra);
> -        return 0;
> +    for (i = 0; i < len / 8; i++) {

You could also do:

  for (i = 0; i < len; i += 8)

then you don't have to multiply by 8 in the line below anymore.

> +        if (!memory_region_access_valid(mr, offset + i * 8, 8, true,
> +                                        MEMTXATTRS_UNSPECIFIED)) {
> +            s390_program_interrupt(env, PGM_OPERAND, ra);
> +            return 0;
> +        }
>      }

Anyway:
Reviewed-by: Thomas Huth <[e-mail]>