Re: [EXT] Re: [virtio-dev] Re: [RFC PATCH] virtio-iommu: Add PAGE_SIZE_MASK property

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Fri, Mar 27, 2020 at 09:17:43AM +0000, Bharat Bhushan wrote:
> 
> Sent again, somehow the email-address got corrupted.
> 
> > -----Original Message-----
> > From: Bharat Bhushan
> > Sent: Friday, March 27, 2020 2:46 PM
> > To: @mx0a-0016f401.pphosted.com
> > Cc: Auger Eric <eric.auger@redhat.com>; virtio-dev@lists.oasis-open.org
> > Subject: RE: [EXT] Re: [virtio-dev] Re: [RFC PATCH] virtio-iommu: Add
> > PAGE_SIZE_MASK property
> > 
> > Hi Jean,
> > 
> > > -----Original Message-----
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Sent: Friday, March 27, 2020 2:31 PM
> > > To: Bharat Bhushan <bbhushan2@marvell.com>
> > > Cc: Auger Eric <eric.auger@redhat.com>;
> > > virtio-dev@lists.oasis-open.org
> > > Subject: Re: [EXT] Re: [virtio-dev] Re: [RFC PATCH] virtio-iommu: Add
> > > PAGE_SIZE_MASK property
> > >
> > > On Fri, Mar 27, 2020 at 05:20:56AM +0000, Bharat Bhushan wrote:
> > > > Hi Jean,
> > > >
> > > > > -----Original Message-----
> > > > > From: Auger Eric <eric.auger@redhat.com>
> > > > > Sent: Thursday, March 26, 2020 4:50 PM
> > > > > To: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > > > Cc: virtio-dev@lists.oasis-open.org; Bharat Bhushan
> > > > > <bbhushan2@marvell.com>
> > > > > Subject: [EXT] Re: [virtio-dev] Re: [RFC PATCH] virtio-iommu: Add
> > > > > PAGE_SIZE_MASK property
> > > > >
> > > > > External Email
> > > > >
> > > > > ------------------------------------------------------------------
> > > > > --
> > > > > --
> > > > > Hi Jean,
> > > > >
> > > > > On 3/26/20 11:49 AM, Jean-Philippe Brucker wrote:
> > > > > > On Mon, Mar 23, 2020 at 03:22:40PM +0100, Auger Eric wrote:
> > > > > >> Hi Jean,
> > > > > >>
> > > > > >> On 3/23/20 2:38 PM, Jean-Philippe Brucker wrote:
> > > > > >>> Add a PROBE property to declare the mapping granularity per endpoint.
> > > > > >>> The virtio-iommu device already declares a granule in its
> > > > > >>> config space, but when endpoints are behind different physical
> > > > > >>> IOMMUs, they may have different mapping granules. This new
> > > > > >>> property allows to override the global page_size_mask for each endpoint.
> > > > > >>>
> > > > > >>> In the future it may be useful to describe more than one
> > > > > >>> page_size_mask for each endpoint, and allow them to negotiate
> > > > > >>> it during ATTACH. For example two masks could allow the driver
> > > > > >>> to choose between 4k and 64k granule, along with their
> > > > > >>> respective block mapping sizes. This could be added by
> > > > > >>> replacing \field{reserved} with an array
> > > > > length, for example.
> > > > > >> Sorry I don't get the use case where several page size bitmaps
> > > > > >> should be exposed.
> > > > > >
> > > > > > For a 4k granule you get block mappings of 2M and 1G. For a 64k
> > > > > > granule you get 512M and 4T block mappings. If you want to
> > > > > > communicate both options to the guest, you need two separate
> > > > > > masks, 0x40201000 and 0x40020010000. Then the guest could choose
> > > > > > one of the granules during attach, if we add a flag to the
> > > > > > attach request. I'm not suggesting we do that now, just trying
> > > > > > to make sure it can be extended if anyone actually wants it.
> > > > > > Personally I don't think it's worth adding, especially given the
> > > > > > additional work required in
> > > the host.
> > > > > OK I get it now.
> > > >
> > > > What some clarification about two page-size-mask configurations available.
> > > >  - Global configuration for page-size-mask
> > > >  - per endpoint page-size-mask configuration
> > > >
> > > > PAGE_SIZE_MASK probe for and endpoint can return zero or non-zero value.
> > > > If it returns non-zero value than it will override the global configuration.
> > > > If PAGE_SIZE_MASK probe for and endpoint return zero value than
> > > > global page-
> > > size-mask configuration will be used.
> > > >
> > > > Is that correct?
> > >
> > > Yes. If a PAGE_SIZE_MASK property is available for an endpoint, the
> > > driver should use that mask. Otherwise it should use the global mask, which is
> > always provided.
> > 
> > That mean even if the device return ZERO as page-size mask it will override global
> > page-size-mask configuration?
> > So device should not return PAGE_SIZE_MASK property when page-size-mask not
> > set for that endpoint, that is zero?
> > 
> > Or
> > 
> > Device can return page-size-mask = 0 for the endpoint in PAGE_SIZE_MASK
> > property, driver have to use global page-size-mask when PAGE_SIZE_MASK
> > property returns page-size-mask = 0.
> > 

Ah sorry I didn't get the question. page_size_mask == 0 isn't valid:

	The device MUST set at least one bit in \field{page_size_mask},
	describing the page granularity. The device MAY set more than one
	bit in \field{page_size_mask}.

So the driver doesn't have to expect a value of 0, that would be a device
bug. If it wants to be on the safe side, then it rejects a page_size_mask
of 0 and use the global mask instead.

> > > I wonder, should we introduce some form of negotiation now?  If the
> > > driver doesn't know about the new probe property, it will use the
> > > global mask. At some point it will send a MAP request not aligned on
> > > the page granule, and the device will abort the request. If instead we
> > > add a flag and page mask field to the attach request, the device would
> > > know that the driver didn't understand the per-endpoint page mask and abort
> > the attach.
> > 
> > What if device support PAGE_SIZE_MASK property  then it will always return the
> > property, with zero or non-zero mask.
> > If zero mask, use global mask otherwise use per-endpoint mask.

My question is about device supports PAGE_SIZE_MASK property and driver
doesn't. For example a linux v4.6 guest would ignore the PAGE_SIZE_MASK
property. Then it will use the global mask, and send MAP requests that
aren't aligned on the per-endpoint page granule (they fail with S_RANGE
status). Should we, with the introduction of the PAGE_SIZE_MASK property,
also introduce a page size negotiation mechanism?  So that the device
knows early whether the guest understands or not the provided
PAGE_SIZE_MASK property?

Thanks,
Jean