Re: [kvm-unit-tests PATCH 1/1] pci: use uint64_t for unsigned long values

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Bill Wendling <morbo@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com,
	alexandru.elisei@arm.com, jmattson@google.com
Subject: Re: [kvm-unit-tests PATCH 1/1] pci: use uint64_t for unsigned long values
Date: Mon, 14 Oct 2019 09:36:54 -0700	[thread overview]
Message-ID: <20191014163654.GC22962@linux.intel.com> (raw)
In-Reply-To: <20191012082623.249497-1-morbo@google.com>

The subject "pci: use uint64_t for unsigned long values" does not reflect
what the patch does.

This should be tagged v2.

Generally speaking, resend the whole series as v2 (or v3, etc...) even if
only one patch is being tweaked.  It's fairly obvious in this case that
the other two other patches from v1 are still relevant, but that won't
always be true.

On Sat, Oct 12, 2019 at 01:26:23AM -0700, Bill Wendling wrote:
> The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> to 32-bit variables. Use 32-bit masks, since we're interested only in
> the least significant 4-bits.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/linux/pci_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> index 1becea8..3bc2b92 100644
> --- a/lib/linux/pci_regs.h
> +++ b/lib/linux/pci_regs.h
> @@ -96,8 +96,8 @@
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
>  #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
> -#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
> -#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
> +#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
> +#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)

The defines themselves should still be 64-bit, e.g. there's a flag right
above to denote a 64-bit address.  Explicitly casting the return value in
pci_bar_mask() where we're dealing with 32-bit addresses would be
preferable.

>  /* bit 1 is reserved if address_space = 1 */
>  
>  /* Header type 0 (normal devices) */
> -- 
> 2.23.0.700.g56cf767bdb-goog
> 