| Message ID | 20171011170205.qpu677qiqe4ludwm@pd.tnic (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 10/11/2017 12:02 PM, Borislav Petkov wrote: ... > > What's with the curly brackets around the case: statements? > I will remove the curly braces. > Anyway, here are some more improvements: > > * you can get rid of the struct copying into out and the bitfields by > doing something like this: > > ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error); > if (ret) > goto e_free; > > /* Clear out reserved fields: */ > data->owner &= BIT(0); > data->config &= BIT(0); > > I'm not sure those are the ones you need to clear but you get > the idea - you simply poke holes in the reserved fields before > copying to userspace. If you need a more sophisticated mask, use > GENMASK/GENMASK_ULL. > If we decide to go with this approach then we need use GENMASK (see below). ... > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -144,11 +144,9 @@ struct sev_data_status { Since the structure is shared with firmware hence I was trying to match it with the spec. > u8 api_major; /* Out */ > u8 api_minor; /* Out */ > u8 state; /* Out */ > - u8 owner : 1; /* Out */ > - u8 reserved1 : 7; > - u32 config : 1; /* Out */ > - u32 reserved2 : 23; > - u32 build : 8; /* Out */ > + u8 owner; /* Out */ This is OK for now. But in future if FW steals another bit from reserved1 field to expose a new flag then 'owner' name will no longer be valid. If you don't to use bit field then we have to use some generic name instead of naming the field as 'owner'. Please note that its not just userspace, KVM driver also uses the same fields and it may also need to know which bit position to use. > + u32 config; /* Out */ > + u32 build; /* Out */ This is a tricky one. The 32-bit are packed as: BIT0 - config.es BIT1-23: reserved BIT24-31: build Now, if we really don't want to use bit field then we have to declare them as: u8 config[3]; u8 build; I would prefer to keep the structure as is. I am OK with changing the userspace sev_user_data_status to match with FW's sev_data_status to avoid the coying from FW structure to userspace structure. > u32 guest_count; /* Out */ > } __packed; > >
On Wed, Oct 11, 2017 at 02:49:55PM -0500, Brijesh Singh wrote: > This is OK for now. But in future if FW steals another bit from reserved1 > field to expose a new flag then 'owner' name will no longer be valid. If you > don't to use bit field then we have to use some generic name instead of > naming the field as 'owner'. Please note that its not just userspace, KVM > driver also uses the same fields and it may also need to know which bit > position to use. So what is this field called in the spec? > This is a tricky one. The 32-bit are packed as: > > BIT0 - config.es > BIT1-23: reserved > BIT24-31: build Is that what the firmware gives? Then it is easy: <firmware_field_name> &= GENMASK(23, 1); and then userspace can pick apart bit 0 and bit24-31. Just use one raw struct which the firmware gives you and the rest is done by the sw. Like clearing reserved fields before copying to user. You don't want to be updating that struct layout later: think of old qemu with new kernel and all those different configurations.
On Wed, Oct 11, 2017 at 10:04:44PM +0200, Borislav Petkov wrote: > Then it is easy: > > <firmware_field_name> &= GENMASK(23, 1); [1:23] range cleared, of course: <firmware_field_name> &= ~GENMASK(23, 1);
On 10/11/2017 03:04 PM, Borislav Petkov wrote: > On Wed, Oct 11, 2017 at 02:49:55PM -0500, Brijesh Singh wrote: >> This is OK for now. But in future if FW steals another bit from reserved1 >> field to expose a new flag then 'owner' name will no longer be valid. If you >> don't to use bit field then we have to use some generic name instead of >> naming the field as 'owner'. Please note that its not just userspace, KVM >> driver also uses the same fields and it may also need to know which bit >> position to use. > > So what is this field called in the spec? > See Section 5.5.2 [1] [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf The bit0 is named as 'owner' and bits 7:1 is left blank (no name is given). >> This is a tricky one. The 32-bit are packed as: >> >> BIT0 - config.es >> BIT1-23: reserved >> BIT24-31: build > > Is that what the firmware gives? > Yes > Then it is easy: > > <firmware_field_name> &= GENMASK(23, 1); > > and then userspace can pick apart bit 0 and bit24-31. > The bit0 is named as config.es and bit1-23 is left blank and bit31-24 is named as build. The current 'struct sev_data_status' matches with the firmware names and the bit fields. Only thing I did was the fields with no name is called as "reservedX" > Just use one raw struct which the firmware gives you and the rest is > done by the sw. Like clearing reserved fields before copying to user. > > You don't want to be updating that struct layout later: think of old > qemu with new kernel and all those different configurations. >
On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote: > The current 'struct sev_data_status' matches with the firmware names and the > bit fields. Only thing I did was the fields with no name is called as > "reservedX" Ok, I see it. So what you actually wanna do is: struct sev_data_status { u8 api_major; /* Out */ u8 api_minor; /* Out */ u8 state; /* Out */ u8 flags; /* Out */ u32 config; /* Out */ u32 guest_count; /* Out */ } __packed; as this is exactly what the firwmare gives you. Theoretically, you could've also done: struct sev_data_status { u64 first_qword; u32 second_dword; }; but you have the fields mostly defined already and that would be too confusing. What I mean is, once you've gotten the command buffer, then you can pick fields apart in software. owner = status.flags & 1; config_es = status.config & 1; build = (status.config >> 24) & 0xff; This way, if new fields get added, you don't have to change the struct definitions - *especially* if they're visible to userspace - and users of that struct can be extended to understand the new fields. And before you copy the struct to userspace, you can simply clear out the reserved fields as nothing should rely on them having a particular value, because, well, they're reserved, doh. Makes sense?
On 10/11/2017 03:28 PM, Borislav Petkov wrote: > On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote: >> The current 'struct sev_data_status' matches with the firmware names and the >> bit fields. Only thing I did was the fields with no name is called as >> "reservedX" > > Ok, I see it. So what you actually wanna do is: > > struct sev_data_status { > u8 api_major; /* Out */ > u8 api_minor; /* Out */ > u8 state; /* Out */ > u8 flags; /* Out */ > u32 config; /* Out */ > u32 guest_count; /* Out */ > } __packed; > OK, if userspace is going to pick bits apart then how about this: struct sev_data_status { u8 api_major; /* Out */ u8 api_minor; /* Out */ u8 state; /* Out */ u32 flags; /* Out */ u8 build; /* Out */ u32 guest_count; /* Out */ } __packed; > > Makes sense? > Please let me know if you are okay with my above structure. -Brijesh
On 10/11/2017 03:45 PM, Brijesh Singh wrote: > > > On 10/11/2017 03:28 PM, Borislav Petkov wrote: >> On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote: >>> The current 'struct sev_data_status' matches with the firmware names >>> and the >>> bit fields. Only thing I did was the fields with no name is called as >>> "reservedX" >> >> Ok, I see it. So what you actually wanna do is: >> >> struct sev_data_status { >> u8 api_major; /* Out */ >> u8 api_minor; /* Out */ >> u8 state; /* Out */ >> u8 flags; /* Out */ >> u32 config; /* Out */ >> u32 guest_count; /* Out */ >> } __packed; >> > > OK, if userspace is going to pick bits apart then how about this: > > struct sev_data_status { > u8 api_major; /* Out */ > u8 api_minor; /* Out */ > u8 state; /* Out */ > u32 flags; /* Out */ > u8 build; /* Out */ > u32 guest_count; /* Out */ > } __packed; > BTW, I kept the build name because KVM driver prints some debug information like this: pr_info_once("SEV: api_major %d api_minor %d build %d\n", status->api_major, status->api_minor, status->build); Of course, I can modify it to access bit field if we decide to go with your recommended structure - thanks
On Wed, Oct 11, 2017 at 03:45:00PM -0500, Brijesh Singh wrote: > OK, if userspace is going to pick bits apart then how about this: > > struct sev_data_status { > u8 api_major; /* Out */ > u8 api_minor; /* Out */ > u8 state; /* Out */ > u32 flags; /* Out */ > u8 build; /* Out */ I guess. Except the spec marks those as 31:24 and belonging to the dword starting at offset 4 and you've made "build" a separate u8 and the dword starts at offset 3. I mean, not a big deal, I think you can change the spec for a change :-) Because it looks like the 4 bytes starting at offset 3 really are meant for miscellaneous bits. And then CONFIG.ES could've been bit 1 of the byte at offset 3. Oh well... > Please let me know if you are okay with my above structure. But yeah, in the end of the day it probably doesn't matter a whole lot. The spec just counts those in 32-bit quantities.
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index d668045956cb..1479db533da0 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -176,9 +176,8 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret) return ret; } -static int sev_ioctl_platform_status(struct sev_issue_cmd *argp) +static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp) { - struct sev_user_data_status out; struct sev_data_status *data; int ret; @@ -186,19 +185,15 @@ static int sev_ioctl_platform_status(struct sev_issue_cmd *argp) if (!data) return -ENOMEM; - ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error); + ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error); if (ret) goto e_free; - out.api_major = data->api_major; - out.api_minor = data->api_minor; - out.state = data->state; - out.owner = data->owner; - out.config = data->config; - out.build = data->build; - out.guest_count = data->guest_count; - if (copy_to_user((void __user *)(uintptr_t) argp->data, - &out, sizeof(struct sev_user_data_status))) + /* Clear out reserved fields: */ + data->owner &= BIT(0); + data->config &= BIT(0); + + if (copy_to_user((void __user *)argp->data, data, sizeof(*data))) ret = -EFAULT; e_free: @@ -226,10 +221,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error); break; - case SEV_PLATFORM_STATUS: { - ret = sev_ioctl_platform_status(&input); + case SEV_PLATFORM_STATUS: + ret = sev_ioctl_do_platform_status(&input); break; - } + default: ret = -EINVAL; goto out; diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 10b843cce75f..223942ba3e7e 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -144,11 +144,9 @@ struct sev_data_status { u8 api_major; /* Out */ u8 api_minor; /* Out */ u8 state; /* Out */ - u8 owner : 1; /* Out */ - u8 reserved1 : 7; - u32 config : 1; /* Out */ - u32 reserved2 : 23; - u32 build : 8; /* Out */ + u8 owner; /* Out */ + u32 config; /* Out */ + u32 build; /* Out */ u32 guest_count; /* Out */ } __packed;