On Thu, 16 Aug 2018 15:38:53 +0200
Harald Freudenberger <freude@xxxxxxxxxxxxx> wrote:
> On 16.08.2018 12:07, Cornelia Huck wrote:
> > One thing on top: What about control domains? The vfio-ap interface
> > allows to assign control domains as well. Do we want to be able to have
> > any control domain available to be used for sending control commands by
> > guests, or do we want to restrict usage of some control domains as well?
> We thought about control domains. The ap code shows the ap_control_domain_mask
> in sysfs (/sys/bus/ap) but this is a read-only attribute and can't be changed
> on LPAR level - needs to be done on the SE.
> When a kvm guest comes into live and the SIE instruction runs the CRYCB
> includes masks for adapter (APM), usage domain (AQM) and control domain (ADM).
> So the vfio device driver needs to set up these mask fields. For the first
> shot we decided to have here the same value as the usage domain mask.
Ok, that sounds like something that can be revisited later.
> > [As an aside: Is 'default' a good name for the driver flag? To a casual
> > reader I think it implies that it is the default driver if no other
> > driver comes along. Call it maybe 'trusted' or something like that?]"
> 'default' or 'trusted' - I am not happy with either of this. Does 'trusted'
> mean the ap driver can't trust the vfio device driver?
ISTR that one motivation for the split was that the normal drivers are
supposed to be those that can be trusted to do secure operations, while
the vfio-ap hands the devices off to a third party.
For a less loaded term: Would something like 'preferred' make sense?
> >> +static ssize_t ap_aqmask_store(struct bus_type *bus, const char *buf,
> >> + size_t count)
> >> +{
> >> + int i;
> >> +
> >> + if (*buf == '+' || *buf == '-') {
> >> + if (kstrtoint(buf, 0, &i))
> >> + return -EINVAL;
> >> + if (i <= -AP_DEVICES || i >= AP_DEVICES)
> >> + return -EINVAL;
> >> + if (mutex_lock_interruptible(&ap_perms_mutex))
> >> + return -ERESTARTSYS;
> >> + if (*buf == '-')
> >> + clear_bit_inv(-i, ap_perms.aqm);
> >> + else
> >> + set_bit_inv(i, ap_perms.aqm);
> >> + } else {
> >> + DECLARE_BITMAP(aqm, AP_DEVICES);
> >> +
> >> + i = hex2bitmap(buf, aqm, AP_DEVICES);
> >> + if (i)
> >> + return i;
> >> + if (mutex_lock_interruptible(&ap_perms_mutex))
> >> + return -ERESTARTSYS;
> >> + for (i = 0; i < AP_DEVICES; i++)
> >> + if (test_bit_inv(i, aqm))
> >> + set_bit_inv(i, ap_perms.aqm);
> >> + else
> >> + clear_bit_inv(i, ap_perms.aqm);
> >> + }
> >> + mutex_unlock(&ap_perms_mutex);
> >> +
> >> + ap_bus_revise_bindings();
> >> +
> >> + return count;
> >> +}
> > There a two things that bother me a bit here:
> > - The sysfs interface accepts both the hex format and the +/- format,
> > but the module parameters only the hex format. Probably not a big
> > deal, but it looks a bit inconsistent.
> Well, does it make sense to have the +/- format for the kernel command line ?
It probably depends on who is setting this up: how do we get from "I
want these adapters for guest usage, the remainder for the host" to the
hex values? I think that question is as valid for the module parameters
as it is for the sysfs interface.
> I'd like to make some improvement here and write a function which is also
> able to handle things like "+3-17" and "+4,-7,+20".
Yes, handling ranges and multiple values makes a lot of sense.
> > - With the more end-user friendly +/- interface, you can always only
> > add/remove one domain/queue at once. For every change, it will walk
> > along the bus and do the reprobing, if needed. It would be more
> > effective if you could trigger all needed changes at once, without
> > being forced to resort to the hex interface for that.
> yes, see above.
Nod.
