Re: [RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions
On Wed, Jul 22, 2020 at 09:47:43AM -0700, Stefano Stabellini wrote:
> On Tue, 21 Jul 2020, Brian Woods wrote:
> > Modify the smmu driver so that it uses the iommu_fwspec helper
> > functions. This means both ARM IOMMU drivers will both use the
> > iommu_fwspec helper functions.
> >
> > Signed-off-by: Brian Woods
>
> [...]
>
> > @@ -1924,14 +1924,21 @@ static int arm_smmu_add_device(struct device *dev)
> > ret = -ENOMEM;
> > goto out_put_group;
> > }
> > + cfg->fwspec = kzalloc(sizeof(struct iommu_fwspec), GFP_KERNEL);
> > + if (!cfg->fwspec) {
> > + kfree(cfg);
> > + ret = -ENOMEM;
> > + goto out_put_group;
> > + }
> > + iommu_fwspec_init(dev, smmu->dev);
>
> Normally the fwspec structure is initialized in
> xen/drivers/passthrough/device_tree.c:iommu_add_dt_device. However here
> we are trying to use it with the legacy bindings, that of course don't
> initialize in iommu_add_dt_device because they are different.
>
> So I imagine this is the reason why we have to initialize iommu_fwspec here
> indepdendently from iommu_add_dt_device.
>
> However, why are we allocating the struct iommu_fwspec twice?
>
> We are calling kzalloc, then iommu_fwspec_init is calling xzalloc.
>
> Similarly, we are storing the pointer to struct iommu_fwspec in
> cfg->fwspec but actually there is already a pointer to struct
> iommu_fwspec in struct device (the one set by dev_iommu_fwspec_set.)
>
> Do we actually need both?
Sorry for taking so long.
Hrm, I've been looking for why I created two fwspecs and I'm not sure
why... It's pretty late, but later this morning I'll try some things
and try and remove it.
Brian
[RFC v2 0/2] Generic SMMU Bindings
Finally had time to do a v2. Changes are in each patch. Brian Woods (2): arm,smmu: switch to using iommu_fwspec functions arm,smmu: add support for generic DT bindings xen/drivers/passthrough/arm/smmu.c| 147 -- xen/drivers/passthrough/device_tree.c | 20 + 2 files changed, 107 insertions(+), 60 deletions(-) -- 2.7.4
[RFC v2 1/2] arm,smmu: switch to using iommu_fwspec functions
Modify the smmu driver so that it uses the iommu_fwspec helper
functions. This means both ARM IOMMU drivers will both use the
iommu_fwspec helper functions.
Signed-off-by: Brian Woods
---
Interested in if combining the legacy and generic bindings paths are
worth or if Xen plans to depreicate legacy bindings at some point.
v1 -> v2
- removed MAX_MASTER_STREAMIDS
- removed unneeded curly brackets
xen/drivers/passthrough/arm/smmu.c| 81 +++
xen/drivers/passthrough/device_tree.c | 3 ++
2 files changed, 47 insertions(+), 37 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 94662a8..7a5c6cd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
/* Xen: The below defines are redefined within the file. Undef it */
@@ -302,9 +303,6 @@ static struct iommu_group *iommu_group_get(struct device
*dev)
/* Start of Linux SMMU code */
-/* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS
-
/* Maximum number of context banks per SMMU */
#define ARM_SMMU_MAX_CBS 128
@@ -597,8 +595,7 @@ struct arm_smmu_smr {
};
struct arm_smmu_master_cfg {
- int num_streamids;
- u16 streamids[MAX_MASTER_STREAMIDS];
+ struct iommu_fwspec *fwspec;
struct arm_smmu_smr *smrs;
};
@@ -779,7 +776,7 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
struct device *dev,
struct of_phandle_args *masterspec)
{
- int i;
+ int i, ret = 0;
struct arm_smmu_master *master;
master = find_smmu_master(smmu, masterspec->np);
@@ -790,34 +787,37 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
return -EBUSY;
}
- if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
- dev_err(dev,
- "reached maximum number (%d) of stream IDs for master
device %s\n",
- MAX_MASTER_STREAMIDS, masterspec->np->name);
- return -ENOSPC;
- }
-
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master)
return -ENOMEM;
+ master->of_node = masterspec->np;
- master->of_node = masterspec->np;
- master->cfg.num_streamids = masterspec->args_count;
+ ret = iommu_fwspec_init(>of_node->dev, smmu->dev);
+ if (ret) {
+ kfree(master);
+ return ret;
+ }
+ master->cfg.fwspec = dev_iommu_fwspec_get(>of_node->dev);
+
+ /* adding the ids here */
+ ret = iommu_fwspec_add_ids(>np->dev,
+ masterspec->args,
+ masterspec->args_count);
+ if (ret)
+ return ret;
/* Xen: Let Xen know that the device is protected by an SMMU */
dt_device_set_protected(masterspec->np);
- for (i = 0; i < master->cfg.num_streamids; ++i) {
- u16 streamid = masterspec->args[i];
-
- if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
-(streamid >= smmu->num_mapping_groups)) {
- dev_err(dev,
- "stream ID for master device %s greater than
maximum allowed (%d)\n",
- masterspec->np->name, smmu->num_mapping_groups);
- return -ERANGE;
+ if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
+ for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
+ if (masterspec->args[i] >= smmu->num_mapping_groups) {
+ dev_err(dev,
+ "stream ID for master device %s greater
than maximum allowed (%d)\n",
+ masterspec->np->name,
smmu->num_mapping_groups);
+ return -ERANGE;
+ }
}
- master->cfg.streamids[i] = streamid;
}
return insert_smmu_master(smmu, master);
}
@@ -1397,15 +1397,15 @@ static int arm_smmu_master_configure_smrs(struct
arm_smmu_device *smmu,
if (cfg->smrs)
return -EEXIST;
- smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL);
+ smrs = kmalloc_array(cfg->fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
if (!smrs) {
dev_err(smmu->dev, "failed to allocate %d SMRs\n",
-
[RFC v2 2/2] arm,smmu: add support for generic DT bindings
Restructure some of the code and add supporting functions for adding
generic device tree (DT) binding support. This will allow for using
current Linux device trees with just modifying the chosen field to
enable Xen.
Signed-off-by: Brian Woods
---
Just realized that I'm fairly sure I need to do some work on the SMRs.
Other than that though, I think things should be okayish.
v1 -> v2
- Corrected how reading of DT is done with generic bindings
xen/drivers/passthrough/arm/smmu.c| 102 +-
xen/drivers/passthrough/device_tree.c | 17 +-
2 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 7a5c6cd..25c090a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -251,6 +251,8 @@ struct iommu_group
atomic_t ref;
};
+static const struct arm_smmu_device *find_smmu(const struct device *dev);
+
static struct iommu_group *iommu_group_alloc(void)
{
struct iommu_group *group = xzalloc(struct iommu_group);
@@ -772,56 +774,104 @@ static int insert_smmu_master(struct arm_smmu_device
*smmu,
return 0;
}
-static int register_smmu_master(struct arm_smmu_device *smmu,
- struct device *dev,
- struct of_phandle_args *masterspec)
+static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
+struct device *dev,
+struct iommu_fwspec *fwspec)
{
- int i, ret = 0;
+ int i;
struct arm_smmu_master *master;
+ struct device_node *dev_node = dev_get_dev_node(dev);
- master = find_smmu_master(smmu, masterspec->np);
+ master = find_smmu_master(smmu, dev_node);
if (master) {
dev_err(dev,
"rejecting multiple registrations for master device
%s\n",
- masterspec->np->name);
+ dev_node->name);
return -EBUSY;
}
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master)
return -ENOMEM;
- master->of_node = masterspec->np;
- ret = iommu_fwspec_init(>of_node->dev, smmu->dev);
- if (ret) {
- kfree(master);
- return ret;
- }
- master->cfg.fwspec = dev_iommu_fwspec_get(>of_node->dev);
-
- /* adding the ids here */
- ret = iommu_fwspec_add_ids(>np->dev,
- masterspec->args,
- masterspec->args_count);
- if (ret)
- return ret;
+ master->of_node = dev_node;
+ master->cfg.fwspec = fwspec;
/* Xen: Let Xen know that the device is protected by an SMMU */
- dt_device_set_protected(masterspec->np);
+ dt_device_set_protected(dev_node);
if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
- for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
- if (masterspec->args[i] >= smmu->num_mapping_groups) {
+ for (i = 0; i < fwspec->num_ids; ++i) {
+ if (fwspec->ids[i] >= smmu->num_mapping_groups) {
dev_err(dev,
"stream ID for master device %s greater
than maximum allowed (%d)\n",
- masterspec->np->name,
smmu->num_mapping_groups);
+ dev_node->name,
smmu->num_mapping_groups);
return -ERANGE;
}
}
}
+
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
+{
+ struct arm_smmu_device *smmu;
+ struct iommu_fwspec *fwspec;
+
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (fwspec == NULL)
+ return -ENXIO;
+
+ smmu = (struct arm_smmu_device *) find_smmu(fwspec->iommu_dev);
+ if (smmu == NULL)
+ return -ENXIO;
+
+ return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+}
+
+static int arm_smmu_dt_xlate_generic(struct device *dev,
+ const struct of_phandle_args *spec)
+{
+ uint32_t mask, fwid = 0;
+
+ if (spec->args_count > 0)
+ fwid |= ((SMR_ID_MASK << SMR_ID_SHIFT) & spec->args[0]) >>
SMR_ID_SHIFT;
+
+ if (spec->args_count > 1)
+ fwid |= ((SMR_MASK_MASK << SMR_MASK_SHIFT) & spec->args[1]) >>
SMR_MASK_SHIFT;
+ else if (!of_property_read_u32(spec->np, "stream-match-mask", ))
+ fwid |=
[Xen-devel] [RFC 2/2] smmu: add support for generic DT bindings
Restructure some of the code and add supporting functions for adding
generic device tree (DT) binding support. The normal add_device and
dt_xlate functions are wrappers of the legacy functions due to legacy
calls needing more arguments because the find_smmu can't a smmu that
isn't initialized.
Signed-off-by: Brian Woods
---
RFC especially on:
- Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate*
functions.
xen/drivers/passthrough/arm/smmu.c| 118 +-
xen/drivers/passthrough/device_tree.c | 17 +
2 files changed, 87 insertions(+), 48 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index c5db5be..08787cd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -251,6 +251,8 @@ struct iommu_group
atomic_t ref;
};
+static const struct arm_smmu_device *find_smmu(const struct device *dev);
+
static struct iommu_group *iommu_group_alloc(void)
{
struct iommu_group *group = xzalloc(struct iommu_group);
@@ -775,64 +777,114 @@ static int insert_smmu_master(struct arm_smmu_device
*smmu,
return 0;
}
-static int register_smmu_master(struct arm_smmu_device *smmu,
- struct device *dev,
- struct of_phandle_args *masterspec)
+/*
+ * Since smmu isn't done initializing before this is run in the legacy
+ * case, create a function where that's passed and then have the generic
+ * function just be a simple wrapper.
+ */
+static int arm_smmu_dt_xlate_legacy(struct device *dev,
+ const struct of_phandle_args *spec,
+ struct iommu_fwspec *fwspec)
+{
+ if ((spec->args_count + fwspec->num_ids) > MAX_MASTER_STREAMIDS) {
+ dev_err(dev,
+ "reached maximum number (%d) of stream IDs for master
device %s\n",
+ MAX_MASTER_STREAMIDS, spec->np->name);
+ return -ENOSPC;
+ }
+
+ /* adding the ids here */
+ return iommu_fwspec_add_ids(dev,
+ spec->args,
+ spec->args_count);
+}
+
+static int arm_smmu_dt_xlate(struct device *dev,
+const struct dt_phandle_args *spec)
+{
+ return arm_smmu_dt_xlate_legacy(dev,
+ spec,
+ dev_iommu_fwspec_get(dev));
+}
+
+static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
+struct device *dev,
+struct iommu_fwspec *fwspec)
{
- int i, ret = 0;
+ int i;
struct arm_smmu_master *master;
+ struct device_node *dev_node = dev_get_dev_node(dev);
+
+ BUG_ON(dev == NULL);
+ BUG_ON(dev_node == NULL);
- master = find_smmu_master(smmu, masterspec->np);
+ master = find_smmu_master(smmu, dev_node);
if (master) {
dev_err(dev,
"rejecting multiple registrations for master device
%s\n",
- masterspec->np->name);
+ dev_node->name);
return -EBUSY;
}
- if (masterspec->args_count > MAX_MASTER_STREAMIDS) {
- dev_err(dev,
- "reached maximum number (%d) of stream IDs for master
device %s\n",
- MAX_MASTER_STREAMIDS, masterspec->np->name);
- return -ENOSPC;
- }
-
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
if (!master) {
return -ENOMEM;
}
- master->of_node = masterspec->np;
-
- ret = iommu_fwspec_init(>of_node->dev, smmu->dev);
- if (ret) {
- kfree(master);
- return ret;
- }
- master->cfg.fwspec = dev_iommu_fwspec_get(>of_node->dev);
+ master->of_node = dev_node;
- /* adding the ids here */
- ret = iommu_fwspec_add_ids(>np->dev,
- masterspec->args,
- masterspec->args_count);
- if (ret)
- return ret;
+ master->cfg.fwspec = fwspec;
/* Xen: Let Xen know that the device is protected by an SMMU */
- dt_device_set_protected(masterspec->np);
+ dt_device_set_protected(dev_node);
if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
- for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
- if (masterspec->args[i] >= smmu->num_mapping_groups) {
+ for (i = 0; i < fwspec->num_ids; ++i) {
+ if (fwspec->ids[i] >= smmu->num_mapping_gro
[Xen-devel] [RFC 0/2] Generic DT Support for SMMU
I'd like some feedback on these patches. Parts I particularly want feedback on are listed below and each patch will have a copy of the relevant parts requesting feedback. Any feedback is welcomed though. Also, the commit messages are a little rough. Patch 1: - Check in device_tree.c. This is needed, otherwise it won't boot due to dev_iommu_fwspec_get(dev) being true and returning EEXIST. I'm not completely sure what type of check is best here. Patch 2: - Checks for the: arm_smmu_dt_add_device* and arm_smmu_dt_xlate* functions. These patches have been tested with device passthrough on a Xilinx ZCU102 with passing the on board ethernet to a guest via the SMMU. Brian Woods (2): arm,smmu: add support for iommu_fwspec functions smmu: add support for generic DT bindings xen/drivers/passthrough/arm/smmu.c| 156 +- xen/drivers/passthrough/device_tree.c | 20 + 2 files changed, 118 insertions(+), 58 deletions(-) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [RFC 1/2] arm, smmu: add support for iommu_fwspec functions
Modify the smmu driver so that it uses the iommu_fwspec helper
functions. This means both ARM IOMMU drivers will both use the
iommu_fwspec helper functions, making enabling generic device tree
bindings in the SMMU driver much cleaner.
Signed-off-by: Brian Woods
---
RFC especially wanted on:
- Check in device_tree.c. This is needed, otherwise it won't boot due
to dev_iommu_fwspec_get(dev) being true and returning EEXIST. I'm
not completely sure what type of check is best here.
xen/drivers/passthrough/arm/smmu.c| 74 ++-
xen/drivers/passthrough/device_tree.c | 3 ++
2 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 94662a8..c5db5be 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
/* Xen: The below defines are redefined within the file. Undef it */
@@ -597,8 +598,7 @@ struct arm_smmu_smr {
};
struct arm_smmu_master_cfg {
- int num_streamids;
- u16 streamids[MAX_MASTER_STREAMIDS];
+ struct iommu_fwspec *fwspec;
struct arm_smmu_smr *smrs;
};
@@ -779,7 +779,7 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
struct device *dev,
struct of_phandle_args *masterspec)
{
- int i;
+ int i, ret = 0;
struct arm_smmu_master *master;
master = find_smmu_master(smmu, masterspec->np);
@@ -798,26 +798,37 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
}
master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
- if (!master)
+ if (!master) {
return -ENOMEM;
+ }
+ master->of_node = masterspec->np;
+
+ ret = iommu_fwspec_init(>of_node->dev, smmu->dev);
+ if (ret) {
+ kfree(master);
+ return ret;
+ }
+ master->cfg.fwspec = dev_iommu_fwspec_get(>of_node->dev);
- master->of_node = masterspec->np;
- master->cfg.num_streamids = masterspec->args_count;
+ /* adding the ids here */
+ ret = iommu_fwspec_add_ids(>np->dev,
+ masterspec->args,
+ masterspec->args_count);
+ if (ret)
+ return ret;
/* Xen: Let Xen know that the device is protected by an SMMU */
dt_device_set_protected(masterspec->np);
- for (i = 0; i < master->cfg.num_streamids; ++i) {
- u16 streamid = masterspec->args[i];
-
- if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
-(streamid >= smmu->num_mapping_groups)) {
- dev_err(dev,
- "stream ID for master device %s greater than
maximum allowed (%d)\n",
- masterspec->np->name, smmu->num_mapping_groups);
- return -ERANGE;
+ if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH)) {
+ for (i = 0; i < master->cfg.fwspec->num_ids; ++i) {
+ if (masterspec->args[i] >= smmu->num_mapping_groups) {
+ dev_err(dev,
+ "stream ID for master device %s greater
than maximum allowed (%d)\n",
+ masterspec->np->name,
smmu->num_mapping_groups);
+ return -ERANGE;
+ }
}
- master->cfg.streamids[i] = streamid;
}
return insert_smmu_master(smmu, master);
}
@@ -1397,15 +1408,15 @@ static int arm_smmu_master_configure_smrs(struct
arm_smmu_device *smmu,
if (cfg->smrs)
return -EEXIST;
- smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL);
+ smrs = kmalloc_array(cfg->fwspec->num_ids, sizeof(*smrs), GFP_KERNEL);
if (!smrs) {
dev_err(smmu->dev, "failed to allocate %d SMRs\n",
- cfg->num_streamids);
+ cfg->fwspec->num_ids);
return -ENOMEM;
}
/* Allocate the SMRs on the SMMU */
- for (i = 0; i < cfg->num_streamids; ++i) {
+ for (i = 0; i < cfg->fwspec->num_ids; ++i) {
int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
smmu->num_mapping_groups);
if (IS_ERR_VALUE(idx)) {
@@ -1416,12 +1427,12 @@ static int arm_smmu_master_configure_smrs(struct
arm_smmu_device *
Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap
On Thu, Oct 17, 2019 at 10:49:15PM +0100, Julien Grall wrote:
> On Thu, 17 Oct 2019 at 22:20, Brian Woods wrote:
> >
> > On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote:
> > > Hi,
> > >
> > > As a user such message would likely put me off. You tell me there are
> > > an overlap, but you don't provide more information even if you likely
> > > have the information in place. However...
> >
> > Well, I suppose the message could be changed to something like:
> > "WARNING: overlap detected in the above memory module addresses."
> > or something to more directly guide the users to the section. Maybe
> > move the 'printk("\n");' after the warning so it's grouped tighter with
> > the module information.
>
> My point stands even for this sort of message. You know the exact
> overlap, so why would you hide it from the users?
We're not hiding it. You're not cluttering up the log with the same
data multiple times. See below.
> > >
> > > ... What's wrong with just dumping the information here directly?
> >
> > IMO, it is better to have all the information printed in one spot.
> > There is less to go through and easier to find out what is happening.
> > There is also the fact that we do not have to print things twice (2 sets
> > of names, starting addresses and ending addresses per overlap) when it
> > is going to be printed in the near future anyway. The cost of this is
> > just one initdata bool, which while I am not thrilled about, does not
> > seem that expensive (compared to a nested loop or printing out at least
> > (16*2 + 12) * 2 characters per overlap(at least on Arm64)).
>
> Again, this is boot code and not a path that is going to be called
> hundreds of time. So performance is the last thing I care in this
> patch.
>
> If we try to help the users by telling them there is an overlap
> between modules, then we should do it properly and tell them the exact
> overlap. Otherwise this is nearly as pointless as a crash later on in
> the boot process.
>
> I also don't want a double for loop or any additional global variable
> when it can be done by simply adding a check in add_boot_module().
This isn't about performance (other than the nested for), this is about
providing a relatively clean and sane log to read. It's not that
difficult to go through the addresses and see conflicts. This also
keeps it all in one part of the log and shorter without losing
information. Shorter and well structured logs (without losing info)
makes it easier to read. Making logs easier to read helps everyone.
Showing the addresses and module name itself will take 2 lines assuming
you stay within 80 chars. (16*2 + 12) * 2 = 88, that's without spaces,
'0x's or any sort of message explaining what's actually going wrong.
The module names and addresses will be printed out anyway in the near
future, so why not group them together?
The purpose of the warning is to tell the user something is wrong, both
messages do that and provide the information to determine what's wrong.
Brian
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap
On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote:
> Hi,
>
> On Thu, 17 Oct 2019 at 21:08, Brian Woods wrote:
> >
> > It's possible for a misconfigured device tree to cause Xen to crash when
> > there are overlapping addresses in the memory modules. Add a warning
> > when printing the addresses to let the user know there's a possible
> > issue.
> >
> > Signed-off-by: Brian Woods
> > ---
> > v1 -> v2
> > - removed nested loop and placed check in add_boot_module()
> >
> > Sample output:
> > ...
> > (XEN) MODULE[0]: 0140 - 01542121 Xen
> > (XEN) MODULE[1]: 03846000 - 03850080 Device Tree
> > (XEN) MODULE[2]: 03853000 - 07fff676 Ramdisk
> > (XEN) MODULE[3]: 0008 - 0318 Kernel
> > (XEN) RESVD[0]: 03846000 - 0385
> > (XEN) RESVD[1]: 03853000 - 07fff676
> > (XEN)
> > (XEN) WARNING: overlap detected in the memory module addresses
> > (XEN)
> > (XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0
> > maxcpus=1 timer_slop=0
> > ...
> >
> > xen/arch/arm/bootfdt.c | 4
> > xen/arch/arm/setup.c| 6 ++
> > xen/include/asm-arm/setup.h | 1 +
> > 3 files changed, 11 insertions(+)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 08fb59f..f8b34d4 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -387,6 +387,10 @@ static void __init early_print_info(void)
> > mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
> > }
> > printk("\n");
> > +
> > +if ( mem_module_overlap )
> > +printk("WARNING: overlap detected in the memory module
> > addresses.\n");
>
> As a user such message would likely put me off. You tell me there are
> an overlap, but you don't provide more information even if you likely
> have the information in place. However...
Well, I suppose the message could be changed to something like:
"WARNING: overlap detected in the above memory module addresses."
or something to more directly guide the users to the section. Maybe
move the 'printk("\n");' after the warning so it's grouped tighter with
the module information.
> > +
> > for ( i = 0 ; i < cmds->nr_mods; i++ )
> > printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
> > cmds->cmdline[i].dt_name,
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 705a917..315a131 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> >
> > domid_t __read_mostly max_init_domid;
> >
> > +bool __initdata mem_module_overlap;
> > +
> > static __used void init_done(void)
> > {
> > /* Must be done past setting system_state. */
> > @@ -254,6 +256,10 @@ struct bootmodule __init
> > *add_boot_module(bootmodule_kind kind,
> > mod->domU = false;
> > return mod;
> > }
> > +
> > +if ( ((mod->start >= start) && (mod->start < start + size)) ||
> > + ((start >= mod->start) && (start < mod->start + mod->size)) )
> > +mem_module_overlap = true;
>
> ... What's wrong with just dumping the information here directly?
IMO, it is better to have all the information printed in one spot.
There is less to go through and easier to find out what is happening.
There is also the fact that we do not have to print things twice (2 sets
of names, starting addresses and ending addresses per overlap) when it
is going to be printed in the near future anyway. The cost of this is
just one initdata bool, which while I am not thrilled about, does not
seem that expensive (compared to a nested loop or printing out at least
(16*2 + 12) * 2 characters per overlap(at least on Arm64)).
I do think the message could use some polish, but this approach makes
the most sense to me.
Brian
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap
It's possible for a misconfigured device tree to cause Xen to crash when
there are overlapping addresses in the memory modules. Add a warning
when printing the addresses to let the user know there's a possible
issue.
Signed-off-by: Brian Woods
---
v1 -> v2
- removed nested loop and placed check in add_boot_module()
Sample output:
...
(XEN) MODULE[0]: 0140 - 01542121 Xen
(XEN) MODULE[1]: 03846000 - 03850080 Device Tree
(XEN) MODULE[2]: 03853000 - 07fff676 Ramdisk
(XEN) MODULE[3]: 0008 - 0318 Kernel
(XEN) RESVD[0]: 03846000 - 0385
(XEN) RESVD[1]: 03853000 - 07fff676
(XEN)
(XEN) WARNING: overlap detected in the memory module addresses
(XEN)
(XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0
maxcpus=1 timer_slop=0
...
xen/arch/arm/bootfdt.c | 4
xen/arch/arm/setup.c| 6 ++
xen/include/asm-arm/setup.h | 1 +
3 files changed, 11 insertions(+)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 08fb59f..f8b34d4 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -387,6 +387,10 @@ static void __init early_print_info(void)
mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
}
printk("\n");
+
+if ( mem_module_overlap )
+printk("WARNING: overlap detected in the memory module addresses.\n");
+
for ( i = 0 ; i < cmds->nr_mods; i++ )
printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
cmds->cmdline[i].dt_name,
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 705a917..315a131 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
domid_t __read_mostly max_init_domid;
+bool __initdata mem_module_overlap;
+
static __used void init_done(void)
{
/* Must be done past setting system_state. */
@@ -254,6 +256,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind
kind,
mod->domU = false;
return mod;
}
+
+if ( ((mod->start >= start) && (mod->start < start + size)) ||
+ ((start >= mod->start) && (start < mod->start + mod->size)) )
+mem_module_overlap = true;
}
mod = >module[mods->nr_mods++];
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 2f8f24e..4bb1ba1 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -122,6 +122,7 @@ void device_tree_get_reg(const __be32 **cell, u32
address_cells,
u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
+extern bool mem_module_overlap;
#endif
/*
* Local variables:
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap
On Thu, Oct 17, 2019 at 10:20:11AM +0100, Julien Grall wrote: > Hi, > > Sorry for the late answer. > > On 11/10/2019 20:07, Brian Woods wrote: > >Which is why I wanted to put it where it was in the patch. Where the > >user would see the warning after the information about the memory > >modules were printed (and fair early). > > I had a think about it, dumping the modules informations before is useful if > you know that you have one module max per kind. So you avoid to print the > modules address/size in the warning. > > However, it is possible to have multiple kernel module (as long as they > don't have the same start address), you could end up with the following > message: > > "WARNING: modules Kernel and Kernel overlap" > > To make the message more meaningful, we would need to print the modules > address/size. Therefore, I don't view that it is important to check > overlapping in early_print_info(). In this case I would favor any code that > don't add a double for loop. Well, adding that information would be easy enough and cheap. It would make it multiline prinktk though: WARNING: memory modules over lap: start_addr-end_addr: modulename start_addr-end_addr: modulename If we're not doing that though, would it make sense to have a initdata bool that checks it in add_boot_module() and then prints a simple warning that there's a memory module overlap in early_print_info()? That way there's no nested for loop and it gets printed where all the addresses get printed (so you can actually figure out where the overlap is). > While thinking about this case, it made me realize that we only check the > start address to consider a match. This means if the size is different, then > it will be ignored. I think we ought to throw at least warning for this case > as well. > > Would you mind to have a look? When you say starting address, do you mean like in the orginal patch? If so, there's no functional change in checking the starts of n on m and m on n then checking the start and end of n on m. > > > >Either way, take your pick of location and if it's only debug or not and > >I can write it up and test it. > > I would still prefer in add_boot_module(). See why above. I wrote I suggested above and tested it so that'll be sent out soon. Brian ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap
On Fri, Oct 11, 2019 at 07:17:29PM +0100, Julien Grall wrote:
> Hi,
>
> On 10/11/19 7:06 PM, Brian Woods wrote:
> >On Fri, Oct 11, 2019 at 05:58:35PM +0100, Julien Grall wrote:
> >For that, you'd need to either check the start and end of the added
> >module or the start of both. So something like:
> >
> >if ( ((mod->start >= start) && (mod->start < (start + size))) ||
> > ((start >= mod->start) && (start < (mod->start + mod->size))) )
> > printk("WARNING: ...");
> >
> >If you don't you run the risk of having a module overlap but not at the
> >start of the added module (unless there's a guaranteed order). You're
> >still running all of the checks from what I can tell, just not in nested
> >for loop so. Plus I'm not sure how many times add_boot_module gets run
> >and the "mod->kind == kind .." if statement gets run and is true.
> >If the above is true, wouldn't that cause extra checks for the for loop
> >iterations before it was true?
>
> For non-dom0less case, we are talking about 4 modules max (Xen, Kernel,
> Initramfs, flask policy). Modules cannot be the shared here.
>
> For dom0less, you are unlikely to have that many domains started from Xen.
> So the number of modules will still be limited (even more if you share it).
Not arguing that. With the second loop (checking two start addresses)
it's only n(n-1)/2 iterations. Even if you had 12 memory modules, it's
only 66 iterations. In the large scheme of things, that isn't THAT
many.
> This code is also only called at boot where there are bigger time consuming
> part (such as domheap initialization). So I would be surprised if you see
> any improvement (other than a couple of cycles) in boot time here.
>
> Therefore, I would favor a readable solution over a micro-optimized solution
> here.
Which is why I wanted to put it where it was in the patch. Where the
user would see the warning after the information about the memory
modules were printed (and fair early).
Either way, take your pick of location and if it's only debug or not and
I can write it up and test it.
Brian
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap
On Fri, Oct 11, 2019 at 05:58:35PM +0100, Julien Grall wrote:
> Hi,
>
> Please at least remove the signature in the e-mail you reply to. The best
> would be to trim the e-mail and answer right below the specific paragraph.
>
> >
> >To make sure the module is going to get added, you'd need to do the
> >check after the for loop. This means there's going to be multiple for
> >loops just spread over the course of adding the boot modules rather than
> >one place.
>
> I don't think you need to do the check after the loop. The only way to go
> out of the loop in add_boot_module() is when i reached mods->nr_mods.
See below.
> >
> >I had this before but decided against it but after changing it to both
> >starts rather than the stand and end (ends look much uglier), it looks
> >cleaner.
> >
> > for ( i = 0 ; i < mods->nr_mods-1; i++ )
> > for ( j = i+1 ; j < mods->nr_mods; j++ )
> > if ( ((mods->module[i].start >= mods->module[j].start) &&
> > (mods->module[i].start <=
> >mods->module[j].start + mods->module[j].size)) ||
> > ((mods->module[j].start >= mods->module[i].start) &&
> > (mods->module[j].start <=
> >mods->module[i].start + mods->module[i].size)) )
> > printk("WARNING: modules %-12s and %-12s overlap\n",
> >boot_module_kind_as_string(mods->module[i].kind),
> >boot_module_kind_as_string(mods->module[j].kind));
> >
> >That's also a possibility.
> >
> >I just don't see a way around it, computationally. You can split where
> >the loops are executed but in the end the same amount of checks/total
> >iterations have to be run.
> >
> >I was talking to someone and he suggested you could just check Xen at
> >early boot and then check other modules later.
>
> What's wrong with:
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 705a917abf..ecd09ec698 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -254,6 +254,10 @@ struct bootmodule __init
> *add_boot_module(bootmodule_kind kind,
> mod->domU = false;
> return mod;
> }
> +
> +if ((mod->start >= start) &&
> +(mod->start < (start + size)))
> +printk("WARNING: modules...\n");
> }
>
> mod = >module[mods->nr_mods++];
>
> Cheers,
>
> --
> Julien Grall
For that, you'd need to either check the start and end of the added
module or the start of both. So something like:
if ( ((mod->start >= start) && (mod->start < (start + size))) ||
((start >= mod->start) && (start < (mod->start + mod->size))) )
printk("WARNING: ...");
If you don't you run the risk of having a module overlap but not at the
start of the added module (unless there's a guaranteed order). You're
still running all of the checks from what I can tell, just not in nested
for loop so. Plus I'm not sure how many times add_boot_module gets run
and the "mod->kind == kind .." if statement gets run and is true.
If the above is true, wouldn't that cause extra checks for the for loop
iterations before it was true?
Brian
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap
On Thu, Oct 10, 2019 at 04:39:07PM +0100, Julien Grall wrote:
> Hi Brian,
>
> Thank you for the patch.
>
> On 10/9/19 8:47 PM, Brian Woods wrote:
> >It's possible for a misconfigured device tree to cause Xen to crash when
> >there are overlapping addresses in the memory modules. Add a warning
> >when printing the addresses to let the user know there's a possible
> >issue when DEBUG is enabled.
> >
> >Signed-off-by: Brian Woods
> >---
> >sample output:
> >...
> >(XEN) MODULE[0]: 0140 - 0153b8f1 Xen
> >(XEN) MODULE[1]: 076d2000 - 076dc080 Device Tree
> >(XEN) MODULE[2]: 076df000 - 07fff364 Ramdisk
> >(XEN) MODULE[3]: 0008 - 0318 Kernel
> >(XEN) RESVD[0]: 076d2000 - 076dc000
> >(XEN) RESVD[1]: 076df000 - 07fff364
> >(XEN)
> >(XEN) WARNING: modules Xen and Kernel overlap
> >(XEN)
> >(XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0
> >maxcpus=1 timer_slop=0
> >...
> >
> > xen/arch/arm/bootfdt.c | 17 +
> > 1 file changed, 17 insertions(+)
> >
> >diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> >index 08fb59f..3cb0c43 100644
> >--- a/xen/arch/arm/bootfdt.c
> >+++ b/xen/arch/arm/bootfdt.c
> >@@ -387,6 +387,23 @@ static void __init early_print_info(void)
> > mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
> > }
> > printk("\n");
> >+
> >+#ifndef NDEBUG
> >+/*
> >+ * Assuming all combinations are checked, only the starting address
> >+ * has to be checked if it's in another memory module's range.
> >+ */
> >+for ( i = 0 ; i < mods->nr_mods; i++ )
> >+for ( j = 0 ; j < mods->nr_mods; j++ )
> >+if ( (i != j) &&
> >+ (mods->module[i].start >= mods->module[j].start) &&
> >+ (mods->module[i].start <
> >+ mods->module[j].start + mods->module[j].size) )
> >+printk("WARNING: modules %-12s and %-12s overlap\n",
> >+ boot_module_kind_as_string(mods->module[i].kind),
> >+ boot_module_kind_as_string(mods->module[j].kind));
>
> I am not entirely happy with the double for-loop here.
>
> As we already go through all the modules in add_boot_module(). Could you
> look whether this check could be part of it?
>
> This should also allow to have this check for non-debug build as well.
>
> Cheers,
>
> --
> Julien Grall
To make sure the module is going to get added, you'd need to do the
check after the for loop. This means there's going to be multiple for
loops just spread over the course of adding the boot modules rather than
one place.
I had this before but decided against it but after changing it to both
starts rather than the stand and end (ends look much uglier), it looks
cleaner.
for ( i = 0 ; i < mods->nr_mods-1; i++ )
for ( j = i+1 ; j < mods->nr_mods; j++ )
if ( ((mods->module[i].start >= mods->module[j].start) &&
(mods->module[i].start <=
mods->module[j].start + mods->module[j].size)) ||
((mods->module[j].start >= mods->module[i].start) &&
(mods->module[j].start <=
mods->module[i].start + mods->module[i].size)) )
printk("WARNING: modules %-12s and %-12s overlap\n",
boot_module_kind_as_string(mods->module[i].kind),
boot_module_kind_as_string(mods->module[j].kind));
That's also a possibility.
I just don't see a way around it, computationally. You can split where
the loops are executed but in the end the same amount of checks/total
iterations have to be run.
I was talking to someone and he suggested you could just check Xen at
early boot and then check other modules later.
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap
It's possible for a misconfigured device tree to cause Xen to crash when
there are overlapping addresses in the memory modules. Add a warning
when printing the addresses to let the user know there's a possible
issue when DEBUG is enabled.
Signed-off-by: Brian Woods
---
sample output:
...
(XEN) MODULE[0]: 0140 - 0153b8f1 Xen
(XEN) MODULE[1]: 076d2000 - 076dc080 Device Tree
(XEN) MODULE[2]: 076df000 - 07fff364 Ramdisk
(XEN) MODULE[3]: 0008 - 0318 Kernel
(XEN) RESVD[0]: 076d2000 - 076dc000
(XEN) RESVD[1]: 076df000 - 07fff364
(XEN)
(XEN) WARNING: modules Xen and Kernel overlap
(XEN)
(XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0
maxcpus=1 timer_slop=0
...
xen/arch/arm/bootfdt.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 08fb59f..3cb0c43 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -387,6 +387,23 @@ static void __init early_print_info(void)
mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
}
printk("\n");
+
+#ifndef NDEBUG
+/*
+ * Assuming all combinations are checked, only the starting address
+ * has to be checked if it's in another memory module's range.
+ */
+for ( i = 0 ; i < mods->nr_mods; i++ )
+for ( j = 0 ; j < mods->nr_mods; j++ )
+if ( (i != j) &&
+ (mods->module[i].start >= mods->module[j].start) &&
+ (mods->module[i].start <
+ mods->module[j].start + mods->module[j].size) )
+printk("WARNING: modules %-12s and %-12s overlap\n",
+ boot_module_kind_as_string(mods->module[i].kind),
+ boot_module_kind_as_string(mods->module[j].kind));
+#endif
+
for ( i = 0 ; i < cmds->nr_mods; i++ )
printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
cmds->cmdline[i].dt_name,
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Fri, Oct 04, 2019 at 10:49:28AM +0100, Julien Grall wrote: > Hi Brian, > > On 04/10/2019 01:25, Brian Woods wrote: > > > >In the log, there's: > >(XEN) MODULE[0]: 0140 - 015328f1 Xen > >(XEN) MODULE[1]: 076d2000 - 076dc080 Device Tree > >(XEN) MODULE[2]: 076df000 - 07fff364 Ramdisk > >(XEN) MODULE[3]: 0008 - 0318 Kernel > >(XEN) RESVD[0]: 076d2000 - 076dc000 > >(XEN) RESVD[1]: 076df000 - 07fff364 > > > >Linux kernel -> 8_ - 318_ > >Xen -> 140_ - 153_28f1 > > > >There's something not quite right here... I'm guessing Xen was working > >at the address before because it was out of the "range" of the Linux > >kernel. Now I guess I need to look into if it's a Xen or u-boot issue. > > The loading address you wrote match the ones you seem to have requested in > U-boot: > > Filename 'yocto-Image'. > Load address: 0x8 > > Filename 'xen-custom.ub'. > Load address: 0x140 > > But the size does not match the one you provided in the Device-Tree: > > Bytes transferred = 18215424 (115f200 hex) > > vs > > 0x0318 - 0x0008 = 0x310 > > This is always a risk when you write in advance the size of the binaries and > location in the Device-Tree. If you are using tftp/load from FS, it is much > less risky to provide a U-boot script that will generate the Xen DT node. > > Cheers, > > -- > Julien Grall Yeah. When I went in and changed the end address in the device tree and it all worked. I'm guessing Xen could use some warnings and some other things to alert the user that the device tree may need tweaking or at the very least some checks. It seems that the blame wasn't primarily on Xen, although it didn't do anyone any favors. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Thu, Oct 03, 2019 at 10:20:36PM +0100, Julien Grall wrote: > Hi Brian, > > On 10/3/19 9:24 PM, Brian Woods wrote: > >On Thu, Oct 03, 2019 at 07:23:23PM +, Julien Grall wrote: > >There's a WARN_ON() between the two debug printks calls I shared above. > > Looking at the log, the MFN seems to correspond to the one right after Xen > (0140 - 015328f1) in memory. > > So it is normal to have the page given to the boot allocator. However, I am > not entirely sure which bit of init_done() is giving the page again to > xenheap. > > It is unlikely to be free_init_memory() because it deal with the init > section that is not at the end of the binary. > > This would leave discard_initial_modules() but there are a check to skip Xen > module. > > The call stack only print the address and not the symbol because it > unregistered the symbols for init. See unregister_init_virtual_memory(). > > (XEN) Xen call trace: > (XEN)[<0021c1a8>] page_alloc.c#free_heap_pages+0x1a8/0x614 (PC) > (XEN)[<0021c1a8>] page_alloc.c#free_heap_pages+0x1a8/0x614 (LR) > (XEN)[<0021e900>] page_alloc.c#init_heap_pages+0x3d4/0x564 > (XEN)[<0021eb24>] init_domheap_pages+0x94/0x9c > (XEN)[<002b83ec>] 002b83ec > (XEN)[<002b8904>] 002b8904 > (XEN)[<00260a3c>] setup.c#init_done+0x10/0x20 > (XEN)[<002b99ac>] 002b99ac > > You should be able to use addr2line on the address with Xen binary. > I have the feeling this will point to discard_initial_modules() as this is > an init function and the symbol should not be printed. > > But, I can't see anything obviously wrong in the function... So I am not > entirely sure what could be the next steps. > > Cheers, > > -- > Julien Grall In the log, there's: (XEN) MODULE[0]: 0140 - 015328f1 Xen (XEN) MODULE[1]: 076d2000 - 076dc080 Device Tree (XEN) MODULE[2]: 076df000 - 07fff364 Ramdisk (XEN) MODULE[3]: 0008 - 0318 Kernel (XEN) RESVD[0]: 076d2000 - 076dc000 (XEN) RESVD[1]: 076df000 - 07fff364 Linux kernel-> 8_ - 318_ Xen -> 140_ - 153_28f1 There's something not quite right here... I'm guessing Xen was working at the address before because it was out of the "range" of the Linux kernel. Now I guess I need to look into if it's a Xen or u-boot issue. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Thu, Oct 03, 2019 at 07:23:23PM +, Julien Grall wrote: > Hi, > > On 03/10/2019 19:15, Brian Woods wrote: > > On Thu, Oct 03, 2019 at 06:08:45PM +0100, Julien Grall wrote: > > (XEN) BW_DEBUG: .6 count_info=0x > > (XEN) Domain heap initialised > > (XEN) BW_DEBUG: 01 count_info=0x0180 > > > > Those debug messages sandwich end_boot_allocator() in start_xen(). > > hmmm, looking back at the thread, the WARN_ON() I suggested is actually > incorrect. :/ Sorry for that. It should be: > > WARN_ON(mfn_x(page_to_mfn(pg + i)) == 0x01533); > > Note the "i" instead of "1". > > If the WARN_ON() is triggered between the two calls, then it would mean > we are giving page to the boot allocator. > > This would imply that next_modules() or dt_unreserved_regions() is not > working as expected (i.e. carving out any modules). > > Also, could you send your log with early printk enabled? > > Cheers, > > -- > Julien Grall Attached are the log and diff. There's a WARN_ON() between the two debug printks calls I shared above. -- Brian Woods PMU Firmware 2019.1 May 25 2019 06:57:33 PMU_ROM Version: xpbr-v8.1.0-0 NOTICE: ATF running on XCZUUNKN/QEMU v4/RTL0.0 at 0xfffea000 NOTICE: BL31: Secure code at 0x6000 NOTICE: BL31: Non secure code at 0x1008 NOTICE: BL31: v2.0(release):xilinx-v2018.3-720-g80d1c790 NOTICE: BL31: Built : 06:54:23, May 25 2019 PMUFW: v1.1 U-Boot 2019.01 (May 25 2019 - 06:55:09 +) Model: ZynqMP ZCU102 Rev1.0 Board: Xilinx ZynqMP DRAM: 4 GiB EL Level: EL2 Chip ID:unknown MMC: mmc@ff17: 0 Loading Environment from SPI Flash... SF: Detected n25q512a with page size 512 Bytes, erase size 128 KiB, total 128 MiB *** Warning - bad CRC, using default environment In:serial@ff00 Out: serial@ff00 Err: serial@ff00 Model: ZynqMP ZCU102 Rev1.0 Board: Xilinx ZynqMP Bootmode: JTAG_MODE Reset reason: EXTERNAL Net: ZYNQ GEM: ff0e, phyaddr c, interface rgmii-id eth0: ethernet@ff0e U-BOOT for xilinx-zcu102-2019_1 BOOTP broadcast 1 DHCP client bound to address 10.0.5.15 (2 ms) Hit any key to stop autoboot: 4 3 0 ZynqMP> setenv serverip 10.0.5.2; tftpb 128 xen-qemu-mod.dtb; tftpb 0x8 yocto-Image; tftpb 140 xen-custom.ub; tftpb 900 yocto-rootfs.cpio.gz.ub; bootm 140 900 128 Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'xen-qemu-mod.dtb'. Load address: 0x128 Loading: *### 6 MiB/s done Bytes transferred = 38019 (9483 hex) Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'yocto-Image'. Load address: 0x8 Loading: *# # # # # # # # # # # # # # # # # # # # 36.6 MiB/s done Bytes transferred = 18215424 (115f200 hex) Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'xen-custom.ub'. Load address: 0x140 Loading: *# 22.4 MiB/s done Bytes transferred = 984464 (f0590 hex) Using ethernet@ff0e d
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Thu, Oct 03, 2019 at 06:08:45PM +0100, Julien Grall wrote:
> Hi,
>
> On 03/10/2019 00:20, Brian Woods wrote:
> >On Wed, Oct 02, 2019 at 02:22:49PM -0700, Brian Woods wrote:
> >>That's odd. I know I copied your and Stefano's email addresses from the
> >>MAINTAINERS file but under my sent emails it shows it has having no
> >>CCs... PEBCAK I guess. My apologies.
> >>
> >>I'll go ahead add those and see if that leads to anything.
> >>
> >>--
> >>Brian Woods
> >
> >Ok, I added:
> > printk("BW_DEBUG: 01 count_info=0x%016lx\n",
> > mfn_to_page(_mfn(0x01533))->count_info);
> >In some places. I'm not sure about some of the earlier ones (the ones
> >before the UART is set up), but all of the ones afterwards that
> >actually get output are:
> > BW_DEBUG: 11 count_info=0x0180
> >
> >Is it worth trying to figure out where the printk buffer is and reading
> >it really early on?
> >
>
> If you haven't enable EARLY_PRINTK in Xen, then you may want to do it. This
> would help you to understand where the page->count_info is not zeroed.
>
>
> Cheers,
>
> --
> Julien Grall
Ah, I'm not used to some of the Arm-isms in Xen yet.
(XEN) BW_DEBUG: .6 count_info=0x
(XEN) Domain heap initialised
(XEN) BW_DEBUG: 01 count_info=0x0180
Those debug messages sandwich end_boot_allocator() in start_xen().
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Wed, Oct 02, 2019 at 02:22:49PM -0700, Brian Woods wrote:
> On Wed, Oct 02, 2019 at 08:59:28PM +0100, Julien Grall wrote:
> > Hi,
> >
> > On 10/2/19 7:56 PM, Brian Woods wrote:
> >
> > Hmmm, the first e-mail didn't land in my inbox directly (I have a filter
> > send to a separate directory any e-mail I not CCed on). Did you BCC me by
> > any change?
> That's odd. I know I copied your and Stefano's email addresses from the
> MAINTAINERS file but under my sent emails it shows it has having no
> CCs... PEBCAK I guess. My apologies.
> >
> > Let see try to troubleshoot it first :).
> >
> > Well, any attachment you send on the ML will store to each subscribers
> > mailbox. I let you do the math here ;)
> >
> > So yeah, pastebin is always the preferred way when you have to send the full
> > log.
> >
> > Thank you for the log. So that's probably not a double-init then.
> >
> > Looking back at the log, the values look quite sane. So I am not entirely
> > sure what is happening.
> >
> > I would check that the frametable is correctly zeroed. You could add a print
> > at the end of setup_frametable_mappings(...) to dump the count_info for the
> > page. Something like:
> > mfn_to_page(_mfn(0x01533))->count_info;
> >
> > If it is correctly initialized, it should be zero.
> >
> > The next step would be to add a similar print in start_xen()
> > (xen/arch/arm/setup.c) and see where the value is not 0 anymore.
> >
> > Cheers,
> >
> > --
> > Julien Grall
>
> I'll go ahead add those and see if that leads to anything.
>
> --
> Brian Woods
Ok, I added:
printk("BW_DEBUG: 01 count_info=0x%016lx\n",
mfn_to_page(_mfn(0x01533))->count_info);
In some places. I'm not sure about some of the earlier ones (the ones
before the UART is set up), but all of the ones afterwards that
actually get output are:
BW_DEBUG: 11 count_info=0x0180
Is it worth trying to figure out where the printk buffer is and reading
it really early on?
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On Wed, Oct 02, 2019 at 08:59:28PM +0100, Julien Grall wrote:
> Hi,
>
> On 10/2/19 7:56 PM, Brian Woods wrote:
> >On 10/2/19 5:52 PM, Julien Grall wrote:
> >>On 10/2/19 1:32 AM, Brian Woods wrote:
> >>>Hello,
> >>
> >>Hi Brian,
> >>
> >>Thank you for report.
> >>
> >>I guess this Arm specific, right? If so, please try to CC
> >>the relevant maintainers and possibly add "arm" in the
> >>subject to avoid any delay (Xen-Devel has quite an high
> >>volume of e-mail!).
> >>
> >>May I also ask to avoiding sending attachment on the mailing
> >>list and instead upload the log somewhere (e.g. pastebin,
> >>your own webserver...)?
> >>
> >
> >I did include all the ARM maintainers, although I forgot to CC
> >Volodymyr. Sorry about that.
>
> Hmmm, the first e-mail didn't land in my inbox directly (I have a filter
> send to a separate directory any e-mail I not CCed on). Did you BCC me by
> any change?
That's odd. I know I copied your and Stefano's email addresses from the
MAINTAINERS file but under my sent emails it shows it has having no
CCs... PEBCAK I guess. My apologies.
> > Also, I'm not sure if this is strictly an
> >ARM or general Xen bug so I left ARM. I guess I should have mentioned
> >that in the email though.
>
> Let see try to troubleshoot it first :).
>
> >
> >I prefer having them as attachments due to the fact I can see everything
> >in mutt. Although if there's a strong community consensus that logs
> >shouldn't be emailed as attachments, I will start using a pastebin like
> >service to post them.
>
> Well, any attachment you send on the ML will store to each subscribers
> mailbox. I let you do the math here ;)
>
> So yeah, pastebin is always the preferred way when you have to send the full
> log.
>
> >
> >>>
> >>>While testing some things out, I found a possible bug in Xen. Xen would
> >>>successfully run when loaded (from u-boot) at some addresses but not
> >>>others. I didn't observe this issue in 4.11 stable, so I did a bisect
> >>>and found that:
> >>>commit f60658c6ae47e74792e6cc48ea2effac8bb52826
> >>>Author: Julien Grall
> >>>Date: Tue Dec 18 13:07:39 2018 +
> >>>
> >>> xen/arm: Stop relocating Xen
> >>>
> >>>was what was causing it to fail when it was loaded to that certain
> >>>address.
> >>
> >>This patch is basically changing how Xen is using the
> >>physical address space. So it exercise more part of Xen
> >>code and most likely a red-herring :).
> >>
> >>However, the logs are quite interesting:
> >>
> >>(XEN) pg[0] MFN 01533 c=0x180 o=0 v=0x7 t=0
> >>
> >>If I am not mistaken, the page state is PGC_state_free.
> >>So this seems to suggest that the page were already
> >>handed over to the allocator.
> >>
> >>Would you mind to apply the patch below and paste the log?
> >>
> >>Hopefully, you see see two WARN_ON() before Xen is crashing.
> >>
> >>Note the patch is assuming the MFN will stay the same after
> >>the patch has been applied. If not, you may need to slightly
> >>tweak it.
> >>
> >>diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> >>index 7cb1bd368b..4bf0dbc727 100644
> >>--- a/xen/common/page_alloc.c
> >>+++ b/xen/common/page_alloc.c
> >>@@ -1389,6 +1389,9 @@ static void free_heap_pages(
> >> for ( i = 0; i < (1 << order); i++ )
> >> {
> >>+
> >>+WARN_ON(mfn_x(page_to_mfn(pg + 1)) == 0x01533);
> >>+
> >> /*
> >> * Cannot assume that count_info == 0, as there are some corner
> >> cases
> >> * where it isn't the case and yet it isn't a bug:
> >>
> >>Cheers,
> >>
> >>--
> >>Julien Grall
> >
> >Attached are the logs of loading patched Xen at the good and bad
> >address. It appears the MFN has stayed the same, although there's only
> >one WARN message for both the good and bad address.
>
> Thank you for the log. So that's probably not a double-init then.
>
> Looking back at the log, the values look quite sane. So I am not entirely
> sure what is happening.
>
> I would check that the frametable is correctly zeroed. You could add a print
> at the end of setup_frametable_mappings(...) to dump the count_info for the
> page. Something like:
> mfn_to_page(_mfn(0x01533))->count_info;
>
> If it is correctly initialized, it should be zero.
>
> The next step would be to add a similar print in start_xen()
> (xen/arch/arm/setup.c) and see where the value is not 0 anymore.
>
> Cheers,
>
> --
> Julien Grall
I'll go ahead add those and see if that leads to anything.
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Errors with Loading Xen at a Certain Address
On 10/2/19 5:52 PM, Julien Grall wrote:
>On 10/2/19 1:32 AM, Brian Woods wrote:
>>Hello,
>
>Hi Brian,
>
>Thank you for report.
>
>I guess this Arm specific, right? If so, please try to CC
>the relevant maintainers and possibly add "arm" in the
>subject to avoid any delay (Xen-Devel has quite an high
>volume of e-mail!).
>
>May I also ask to avoiding sending attachment on the mailing
>list and instead upload the log somewhere (e.g. pastebin,
>your own webserver...)?
>
I did include all the ARM maintainers, although I forgot to CC
Volodymyr. Sorry about that. Also, I'm not sure if this is strictly an
ARM or general Xen bug so I left ARM. I guess I should have mentioned
that in the email though.
I prefer having them as attachments due to the fact I can see everything
in mutt. Although if there's a strong community consensus that logs
shouldn't be emailed as attachments, I will start using a pastebin like
service to post them.
>>
>>While testing some things out, I found a possible bug in Xen. Xen would
>>successfully run when loaded (from u-boot) at some addresses but not
>>others. I didn't observe this issue in 4.11 stable, so I did a bisect
>>and found that:
>>commit f60658c6ae47e74792e6cc48ea2effac8bb52826
>>Author: Julien Grall
>>Date: Tue Dec 18 13:07:39 2018 +
>>
>> xen/arm: Stop relocating Xen
>>
>>was what was causing it to fail when it was loaded to that certain
>>address.
>
>This patch is basically changing how Xen is using the
>physical address space. So it exercise more part of Xen
>code and most likely a red-herring :).
>
>However, the logs are quite interesting:
>
>(XEN) pg[0] MFN 01533 c=0x180 o=0 v=0x7 t=0
>
>If I am not mistaken, the page state is PGC_state_free.
>So this seems to suggest that the page were already
>handed over to the allocator.
>
>Would you mind to apply the patch below and paste the log?
>
>Hopefully, you see see two WARN_ON() before Xen is crashing.
>
>Note the patch is assuming the MFN will stay the same after
>the patch has been applied. If not, you may need to slightly
>tweak it.
>
>diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>index 7cb1bd368b..4bf0dbc727 100644
>--- a/xen/common/page_alloc.c
>+++ b/xen/common/page_alloc.c
>@@ -1389,6 +1389,9 @@ static void free_heap_pages(
> for ( i = 0; i < (1 << order); i++ )
> {
>+
>+WARN_ON(mfn_x(page_to_mfn(pg + 1)) == 0x01533);
>+
> /*
> * Cannot assume that count_info == 0, as there are some corner cases
> * where it isn't the case and yet it isn't a bug:
>
>Cheers,
>
>--
>Julien Grall
Attached are the logs of loading patched Xen at the good and bad
address. It appears the MFN has stayed the same, although there's only
one WARN message for both the good and bad address.
--
Brian Woods
PMU Firmware 2019.1 May 25 2019 06:57:33
PMU_ROM Version: xpbr-v8.1.0-0
NOTICE: ATF running on XCZUUNKN/QEMU v4/RTL0.0 at 0xfffea000
NOTICE: BL31: Secure code at 0x6000
NOTICE: BL31: Non secure code at 0x1008
NOTICE: BL31: v2.0(release):xilinx-v2018.3-720-g80d1c790
NOTICE: BL31: Built : 06:54:23, May 25 2019
PMUFW: v1.1
U-Boot 2019.01 (May 25 2019 - 06:55:09 +)
Model: ZynqMP ZCU102 Rev1.0
Board: Xilinx ZynqMP
DRAM: 4 GiB
EL Level: EL2
Chip ID:unknown
MMC: mmc@ff17: 0
Loading Environment from SPI Flash... SF: Detected n25q512a with page size 512
Bytes, erase size 128 KiB, total 128 MiB
*** Warning - bad CRC, using default environment
In:serial@ff00
Out: serial@ff00
Err: serial@ff00
Model: ZynqMP ZCU102 Rev1.0
Board: Xilinx ZynqMP
Bootmode: JTAG_MODE
Reset reason: EXTERNAL
Net: ZYNQ GEM: ff0e, phyaddr c, interface rgmii-id
eth0: ethernet@ff0e
U-BOOT for xilinx-zcu102-2019_1
BOOTP broadcast 1
DHCP client bound to address 10.0.5.15 (1 ms)
Hit any key to stop autoboot: 4 3 0
ZynqMP> setenv serverip 10.0.5.2; tftpb 128 xen-qemu-mod.dtb; tftpb 0x8
yocto-Image; tftpb 420 xen-custom-patched-ga.ub; tftpb 900
yocto-rootfs.cpio.gz.ub; bootm 420 900 128
Using ethernet@ff0e device
TFTP from server 10.0.5.2; our IP address is 10.0.5.15
F
[Xen-devel] Errors with Loading Xen at a Certain Address
Hello, While testing some things out, I found a possible bug in Xen. Xen would successfully run when loaded (from u-boot) at some addresses but not others. I didn't observe this issue in 4.11 stable, so I did a bisect and found that: commit f60658c6ae47e74792e6cc48ea2effac8bb52826 Author: Julien Grall Date: Tue Dec 18 13:07:39 2018 + xen/arm: Stop relocating Xen was what was causing it to fail when it was loaded to that certain address. I've attached the logs from a build from staging (about a week or so ago) with both a passing and failing address. -- Brian Woods PMU Firmware 2019.1 May 25 2019 06:57:33 PMU_ROM Version: xpbr-v8.1.0-0 NOTICE: ATF running on XCZUUNKN/QEMU v4/RTL0.0 at 0xfffea000 NOTICE: BL31: Secure code at 0x6000 NOTICE: BL31: Non secure code at 0x1008 NOTICE: BL31: v2.0(release):xilinx-v2018.3-720-g80d1c790 NOTICE: BL31: Built : 06:54:23, May 25 2019 PMUFW: v1.1 U-Boot 2019.01 (May 25 2019 - 06:55:09 +) Model: ZynqMP ZCU102 Rev1.0 Board: Xilinx ZynqMP DRAM: 4 GiB EL Level: EL2 Chip ID:unknown MMC: mmc@ff17: 0 Loading Environment from SPI Flash... SF: Detected n25q512a with page size 512 Bytes, erase size 128 KiB, total 128 MiB *** Warning - bad CRC, using default environment In:serial@ff00 Out: serial@ff00 Err: serial@ff00 Model: ZynqMP ZCU102 Rev1.0 Board: Xilinx ZynqMP Bootmode: JTAG_MODE Reset reason: EXTERNAL Net: ZYNQ GEM: ff0e, phyaddr c, interface rgmii-id eth0: ethernet@ff0e U-BOOT for xilinx-zcu102-2019_1 BOOTP broadcast 1 DHCP client bound to address 10.0.5.15 (1 ms) Hit any key to stop autoboot: 4 0 ZynqMP> setenv serverip 10.0.5.2; tftpb 128 xen-qemu-mod.dtb; tftpb 0x8 yocto-Image; tftpb 420 xen-custom.ub; tftpb 900 yocto-rootfs.cpio.gz.ub; bootm 420 900 128 Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'xen-qemu-mod.dtb'. Load address: 0x128 Loading: *### 12.1 MiB/s done Bytes transferred = 38019 (9483 hex) Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'yocto-Image'. Load address: 0x8 Loading: *# # # # # # # # # # # # # # # # # # # # 32.9 MiB/s done Bytes transferred = 18215424 (115f200 hex) Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'xen-custom.ub'. Load address: 0x420 Loading: *# 32.4 MiB/s done Bytes transferred = 984464 (f0590 hex) Using ethernet@ff0e device TFTP from server 10.0.5.2; our IP address is 10.0.5.15 Filename 'yocto-rootfs.cpio.gz.ub'. Load address: 0x900 Loading: *# # # # # # # # # # # 32.7 MiB/s d
Re: [Xen-devel] [PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR
On Thu, Aug 30, 2018 at 09:49:58AM -0600, Jan Beulich wrote:
> >>> On 27.08.18 at 18:55, wrote:
> > Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
> > AMD CPUs. There needs to be locking logic for family 17h with SMT
> > enabled since both threads share the same MSR. Otherwise, a core just
> > needs to write to the LS_CFG MSR. For more information see:
> > https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreB
> >
> > ypassDisable_Whitepaper_final.pdf
> >
> > Signed-off-by: Brian Woods
> > ---
> > xen/arch/x86/cpu/amd.c| 13 +--
> > xen/arch/x86/smpboot.c| 3 +
> > xen/arch/x86/spec_ctrl.c | 172
> > +-
> > xen/include/asm-x86/cpufeatures.h | 1 +
> > xen/include/asm-x86/spec_ctrl.h | 2 +
> > 5 files changed, 181 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> > index 6e65ae7427..e96f14268e 100644
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -601,8 +601,8 @@ static void init_amd(struct cpuinfo_x86 *c)
> > }
> >
> > /*
> > -* If the user has explicitly chosen to disable Memory Disambiguation
> > -* to mitigiate Speculative Store Bypass, poke the appropriate MSR.
> > +* Poke the LS_CFG MSR to see if the mitigation for Speculative
> > +* Store Bypass is available.
> > */
> > if (!ssbd_amd_ls_cfg_mask) {
> > int bit = -1;
> > @@ -615,14 +615,9 @@ static void init_amd(struct cpuinfo_x86 *c)
> >
> > if (bit >= 0)
> > ssbd_amd_ls_cfg_mask = 1ull << bit;
> > - }
> >
> > - if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> > - ssbd_amd_ls_cfg_av = true;
> > - if (opt_ssbd) {
> > - value |= ssbd_amd_ls_cfg_mask;
> > - wrmsr_safe(MSR_AMD64_LS_CFG, value);
> > - }
> > + if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG,
> > value))
> > + ssbd_amd_ls_cfg_av = true;
> > }
> >
> > /* MFENCE stops RDTSC speculation */
> > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> > index 7e76cc3d68..b103b46dee 100644
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -376,6 +376,9 @@ void start_secondary(void *unused)
> > if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> > wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
> >
> > +if ( default_xen_ssbd_amd_ls_cfg_en )
> > +ssbd_amd_ls_cfg_set(true);
> > +
> > if ( xen_guest )
> > hypervisor_ap_setup();
> >
> > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> > index b32c12c6c0..89cc444f56 100644
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
> > @@ -20,6 +20,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly
> > l1tf_safe_maddr;
> > static bool __initdata cpu_has_bug_l1tf;
> > static unsigned int __initdata l1d_maxphysaddr;
> >
> > +/* for SSBD support for AMD via LS_CFG */
> > +#define SSBD_AMD_MAX_SOCKET 4
>
> Oh, went up from 2 to 4? But still not a dynamic upper bound?
>
Well, 4 was just to be safe. 2 is more than reasonable IMO. Having it
dynamic seems like a lot of work to save 8 bytes (or after the bump to
4, 24 bytes) when it doesn't get used.
> > +struct ssbd_amd_ls_cfg_smt_status {
> > +spinlock_t lock;
> > +uint32_t mask;
> > +} __attribute__ ((aligned (64)));
>
> Ehem. See the respective comment on patch 1. To put it bluntly,
> I'm not willing to look at patches where prior comments were not
> addressed, the more that you had verbally agreed to use
> SMP_CACHE_BYTES here.
>
> Jan
>
Well, a rebasing failed and I had to regenerate the patches from a
diff of the v3 patches combined, and I missed fixing that one part.
I'm terrible sorry I missed that one fix.
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR
Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
AMD CPUs. There needs to be locking logic for family 17h with SMT
enabled since both threads share the same MSR. Otherwise, a core just
needs to write to the LS_CFG MSR. For more information see:
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c| 13 +--
xen/arch/x86/smpboot.c| 3 +
xen/arch/x86/spec_ctrl.c | 172 +-
xen/include/asm-x86/cpufeatures.h | 1 +
xen/include/asm-x86/spec_ctrl.h | 2 +
5 files changed, 181 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 6e65ae7427..e96f14268e 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -601,8 +601,8 @@ static void init_amd(struct cpuinfo_x86 *c)
}
/*
-* If the user has explicitly chosen to disable Memory Disambiguation
-* to mitigiate Speculative Store Bypass, poke the appropriate MSR.
+* Poke the LS_CFG MSR to see if the mitigation for Speculative
+* Store Bypass is available.
*/
if (!ssbd_amd_ls_cfg_mask) {
int bit = -1;
@@ -615,14 +615,9 @@ static void init_amd(struct cpuinfo_x86 *c)
if (bit >= 0)
ssbd_amd_ls_cfg_mask = 1ull << bit;
- }
- if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
- ssbd_amd_ls_cfg_av = true;
- if (opt_ssbd) {
- value |= ssbd_amd_ls_cfg_mask;
- wrmsr_safe(MSR_AMD64_LS_CFG, value);
- }
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG,
value))
+ ssbd_amd_ls_cfg_av = true;
}
/* MFENCE stops RDTSC speculation */
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7e76cc3d68..b103b46dee 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -376,6 +376,9 @@ void start_secondary(void *unused)
if ( boot_cpu_has(X86_FEATURE_IBRSB) )
wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+if ( default_xen_ssbd_amd_ls_cfg_en )
+ssbd_amd_ls_cfg_set(true);
+
if ( xen_guest )
hypervisor_ap_setup();
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index b32c12c6c0..89cc444f56 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
@@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly
l1tf_safe_maddr;
static bool __initdata cpu_has_bug_l1tf;
static unsigned int __initdata l1d_maxphysaddr;
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 4
+struct ssbd_amd_ls_cfg_smt_status {
+spinlock_t lock;
+uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en;
+bool __read_mostly default_xen_ssbd_amd_ls_cfg_en;
bool ssbd_amd_ls_cfg_av;
uint64_t __read_mostly ssbd_amd_ls_cfg_mask;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET];
static int __init parse_bti(const char *s)
{
@@ -319,7 +329,7 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
!ssbd_amd_ls_cfg_av ? "" :
- opt_ssbd ? " LS_CFG_SSBD+" : "
LS_CFG_SSBD-",
+ default_xen_ssbd_amd_ls_cfg_en? " LS_CFG_SSBD+" : "
LS_CFG_SSBD-",
opt_ibpb ? " IBPB" : "",
opt_l1d_flush ? " L1D_FLUSH" : "");
@@ -725,6 +735,162 @@ static __init int parse_xpti(const char *s)
}
custom_param("xpti", parse_xpti);
+/*
+ * Enabling SSBD on AMD processers via the LS_CFG MSR
+ *
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just setting an MSR bit. For 17h, it depends
+ * if SMT is enabled. If SMT, are two threads that share a single MSR
+ * so there needs to be a lock and a virtual bit for each thread,
+ * otherwise it's the same as family 15h/16h.
+ */
+
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+uint64_t ls_cfg, new_ls_cfg;
+
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+
+if ( enable_ssbd )
+new_ls_cfg = ls_cfg | ssbd_amd_ls_cfg_mask;
+else
+new_ls_cfg = ls_cfg & ~ssbd_amd_ls_cfg_mask;
+
+if ( new_ls_cfg != ls_cfg )
+wrmsrl(MSR_AMD64_LS_CFG, new_ls_cfg);
+}
+
+static void ssbd_amd_ls_c
[Xen-devel] [PATCH v3 0/2] SSBD via LS_CFG
This series of patches is for enabling SSBD via the LS_CFG MSR for family 15h-17h. The first patch make it so that the information is correctly displayed on boot. The last patch is for further enablement for Xen switching SSBD on or off internally, or for further control of SSBD for guests via the VIRT_SPEC_CTRL. Note: Patch 1 is standalone and just improves reporting of SSBD in the init print statements. Patch 2 is intended for a later date when there's a C ALTERNATIVE (which I've talked to Andy about some) and will most likely get rolled into series of patches for giving HVM guests control of SSBD when it's only available via LS_CFG. v1 -> v2: - changed some variable types - updated ssbd_amd_ls_cfg_set_nonsmt function - changed sbd_amd_ls_cfg_init to a presmp_initcall - deleted ssbd_amd_ls_cfg_set_init due to ^ and it happening later in the boot - various smaller cleanups v2 -> v3: - moved the SSBD_AMD_LS_CFG synthetic feature from patch 1 to 2 and added ssbd_amd_ls_cfg_av for it's use in patch 1 - in patch 2, only set SSBD_AMD_LS_CFG once everything is known to be ready/good - since it's later in the boot process due to a v1 change, use nr_sockets - various smaller changes/cleanups from Jan's feedback Brian Woods (2): x86/spec-ctrl: add AMD SSBD LS_CFG in init print x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR xen/arch/x86/cpu/amd.c| 15 ++-- xen/arch/x86/smpboot.c| 3 + xen/arch/x86/spec_ctrl.c | 180 +- xen/include/asm-x86/cpufeatures.h | 1 + xen/include/asm-x86/spec_ctrl.h | 5 ++ 5 files changed, 195 insertions(+), 9 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 1/2] x86/spec-ctrl: add AMD SSBD LS_CFG in init print
Edit the initialization code for AMD's SSBD via the LS_CFG MSR and
enable it to pass the status to the initial spec-ctrl print_details at
boot.
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c | 12 +---
xen/arch/x86/spec_ctrl.c| 10 --
xen/include/asm-x86/spec_ctrl.h | 3 +++
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index a7afa2fa7a..6e65ae7427 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -604,7 +604,7 @@ static void init_amd(struct cpuinfo_x86 *c)
* If the user has explicitly chosen to disable Memory Disambiguation
* to mitigiate Speculative Store Bypass, poke the appropriate MSR.
*/
- if (opt_ssbd) {
+ if (!ssbd_amd_ls_cfg_mask) {
int bit = -1;
switch (c->x86) {
@@ -613,8 +613,14 @@ static void init_amd(struct cpuinfo_x86 *c)
case 0x17: bit = 10; break;
}
- if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
- value |= 1ull << bit;
+ if (bit >= 0)
+ ssbd_amd_ls_cfg_mask = 1ull << bit;
+ }
+
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ ssbd_amd_ls_cfg_av = true;
+ if (opt_ssbd) {
+ value |= ssbd_amd_ls_cfg_mask;
wrmsr_safe(MSR_AMD64_LS_CFG, value);
}
}
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index c430b25b84..b32c12c6c0 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -58,6 +58,9 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly
l1tf_safe_maddr;
static bool __initdata cpu_has_bug_l1tf;
static unsigned int __initdata l1d_maxphysaddr;
+bool ssbd_amd_ls_cfg_av;
+uint64_t __read_mostly ssbd_amd_ls_cfg_mask;
+
static int __init parse_bti(const char *s)
{
const char *ss;
@@ -281,11 +284,12 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
printk("Speculative mitigation facilities:\n");
/* Hardware features which pertain to speculative mitigations. */
-printk(" Hardware features:%s%s%s%s%s%s%s%s%s%s\n",
+printk(" Hardware features:%s%s%s%s%s%s%s%s%s%s%s\n",
(_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_SSBD)) ? " SSBD" : "",
+ ssbd_amd_ls_cfg_av ? " LS_CFG_SSBD" : "",
(e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "",
(caps & ARCH_CAPABILITIES_IBRS_ALL) ? " IBRS_ALL" : "",
(caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : "",
@@ -305,7 +309,7 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
"\n");
/* Settings for Xen's protection, irrespective of guests. */
-printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s%s\n",
+printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE? "LFENCE" :
@@ -314,6 +318,8 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_IBRS) ? "IBRS+" : "IBRS-",
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
+ !ssbd_amd_ls_cfg_av ? "" :
+ opt_ssbd ? " LS_CFG_SSBD+" : "
LS_CFG_SSBD-",
opt_ibpb ? " IBPB" : "",
opt_l1d_flush ? " L1D_FLUSH" : "");
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 8f8aad40bb..1b9101a988 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -50,6 +50,9 @@ extern int8_t opt_pv_l1tf;
*/
extern paddr_t l1tf_addr_mask, l1tf_safe_maddr;
+extern bool ssbd_amd_ls_cfg_av;
+extern uint64_t ssbd_amd_ls_cfg_mask;
+
static inline void init_shadow_spec_ctrl_state(void)
{
struct cpu_info *info = get_cpu_info();
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
rn"? > > > > Because it forces all the failed returns down one code path. I can > > change it if you wish. > > If any cleanup is to be done, using "goto" for this purpose is > generally accepted (although personally I dislike "goto" in > general). Here, however, nothing has been done yet and the > cleanup path is non-trivial. It took me extra time to verify > that nothing bad would happen from going that path despite > nothing having been done before. It is far more clear to the > reader imo if there is just "return" here. Well, it's just a difference of opinion at this point. I'll change it. > >> > +default_xen_ssbd_amd_ls_cfg_en = false; > >> > + > >> > +dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to > >> > errors\n"); > >> > + > >> > +return 1; > >> > >> If the function returns 0 and 1 only, it looks like you've meant to > >> use bool, false, and true respectively. > > > > Because it's more of an error code than boolean logic value. I can > > switch it over to bool if you want. > > Error code style returns would please use (negative) errno-style > numbers. But if the function really just means to say "success" > or "failure", without particular error codes, then boolean would > imo still be preferable. > > Jan > Sounds fair to change it to bool. I'll do that. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86: use VMLOAD for PV context switch
On Fri, Aug 17, 2018 at 01:33:43AM -0600, Jan Beulich wrote: > >>> On 17.08.18 at 00:04, wrote: > > On Tue, Jul 10, 2018 at 04:14:11AM -0600, Jan Beulich wrote: > >> Having noticed that VMLOAD alone is about as fast as a single of the > >> involved WRMSRs, I thought it might be a reasonable idea to also use it > >> for PV. Measurements, however, have shown that an actual improvement can > >> be achieved only with an early prefetch of the VMCB (thanks to Andrew > >> for suggesting to try this), which I have to admit I can't really > >> explain. This way on my Fam15 box context switch takes over 100 clocks > >> less on average (the measured values are heavily varying in all cases, > >> though). > >> > >> This is intentionally not using a new hvm_funcs hook: For one, this is > >> all about PV, and something similar can hardly be done for VMX. > >> Furthermore the indirect to direct call patching that is meant to be > >> applied to most hvm_funcs hooks would be ugly to make work with > >> functions having more than 6 parameters. > >> > >> Signed-off-by: Jan Beulich > > > > I have confirmed it with a senior hardware engineer and using vmload in > > this fashion is safe and recommended for performance. As far as using > > vmload with PV. > > > > Acked-by: Brian Woods > > Thanks. There's another aspect in this same area that I'd like to > improve, and hence seek clarification on up front: Currently SVM > code uses two pages per CPU, one for host_vmcb and the other > for hsa. Afaict the two uses are entirely dis-joint: The host save > area looks to be simply yet another VMCB, and the parts accessed > during VMRUN / VM exit are fully separate from the ones used by > VMLOAD / VMSAVE. Therefore I think both could be folded, > reducing code size as well as memory (and perhaps cache) footprint. > > I think this separation was done because the PM mentions both > data structures separately, but iirc there's nothing said anywhere > that the two structures indeed need to be distinct. > > Jan From APM Vol 2 15.30.4 VM_HSAVE_PA MSR (C001_0117h) The 64-bit read/write VM_HSAVE_PA MSR holds the physical address of a 4KB block of memory where VMRUN saves host state, and from which #VMEXIT reloads host state. The VMM software is expected to set up this register before issuing the first VMRUN instruction. Software must not attempt to read or write the host save-state area directly. Writing this MSR causes a #GP if: • any of the low 12 bits of the address written are nonzero, or • the address written is greater than or equal to the maximum supported physical address for this implementation. It seems that the HSA is needed for the state of the guest/host. I don't see how they can be folded in together. Am I missing something? -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86: use VMLOAD for PV context switch
On Tue, Jul 10, 2018 at 04:14:11AM -0600, Jan Beulich wrote: > Having noticed that VMLOAD alone is about as fast as a single of the > involved WRMSRs, I thought it might be a reasonable idea to also use it > for PV. Measurements, however, have shown that an actual improvement can > be achieved only with an early prefetch of the VMCB (thanks to Andrew > for suggesting to try this), which I have to admit I can't really > explain. This way on my Fam15 box context switch takes over 100 clocks > less on average (the measured values are heavily varying in all cases, > though). > > This is intentionally not using a new hvm_funcs hook: For one, this is > all about PV, and something similar can hardly be done for VMX. > Furthermore the indirect to direct call patching that is meant to be > applied to most hvm_funcs hooks would be ugly to make work with > functions having more than 6 parameters. > > Signed-off-by: Jan Beulich I have confirmed it with a senior hardware engineer and using vmload in this fashion is safe and recommended for performance. As far as using vmload with PV. Acked-by: Brian Woods -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
> > +if ( (ls_cfg & ssbd_amd_ls_cfg_mask) && (status->mask == 0) )
>
> Please prefer the shorter ! over " == 0".
>
> > +{
> > +ls_cfg &= ~ssbd_amd_ls_cfg_mask;
> > +wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> > +}
> > +}
> > +}
> > +
> > +spin_unlock(>lock);
> > +}
> > +
> > +void ssbd_amd_ls_cfg_set(bool enable_ssbd)
> > +{
> > +if ( !ssbd_amd_ls_cfg_mask ||
> > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
> > +dprintk(XENLOG_ERR, "SSBD AMD LS CFG: invalid mask or missing
> > feature\n");
>
> If the plan is for the function to also be called at runtime eventually,
> this dprintk() needs to go away.
>
> > +return;
> > +}
> > +
> > +if ( ssbd_amd_smt_en )
> > +ssbd_amd_ls_cfg_set_smt(enable_ssbd);
> > +else
> > +ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
> > +}
> > +
> > +static int __init ssbd_amd_ls_cfg_init(void)
> > +{
> > +uint32_t cores_per_socket, threads_per_core;
> > +const struct cpuinfo_x86 *c = _cpu_data;
> > +uint32_t core, socket;
> > +
> > +if ( !ssbd_amd_ls_cfg_mask ||
> > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> > +goto ssbd_amd_ls_cfg_init_fail;
>
> Why not simply "return"?
Because it forces all the failed returns down one code path. I can
change it if you wish.
> > +switch ( c->x86 )
> > +{
> > +case 0x15:
> > +case 0x16:
> > +break;
> > +
> > +case 0x17:
> > +cores_per_socket = c->x86_max_cores;
> > +threads_per_core = c->x86_num_siblings;
> > +
> > +if ( threads_per_core > 1 )
> > +{
> > +ssbd_amd_smt_en = true;
> > +for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> > +{
> > +ssbd_amd_smt_status[socket] =
> > + (struct ssbd_amd_ls_cfg_smt_status *)
> > + xmalloc_array(struct ssbd_amd_ls_cfg_smt_status,
> > +cores_per_socket);
>
> Pointless cast.
>
> > +if ( ssbd_amd_smt_status[socket] == NULL )
> > +{
> > +dprintk(XENLOG_ERR,
> > +"SSBD AMD LS CFG: error in status allocing\n");
> > +goto ssbd_amd_ls_cfg_init_fail;
> > +}
> > +}
> > +
> > +for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> > +{
> > +for ( core = 0; core < cores_per_socket; core++ )
> > +{
> > +
> > spin_lock_init(_amd_smt_status[socket][core].lock);
> > +ssbd_amd_smt_status[socket][core].mask = 0;
> > +}
> > +}
> > +}
> > +break;
> > +
> > +default:
> > +goto ssbd_amd_ls_cfg_init_fail;
> > +}
> > +
> > +if ( default_xen_ssbd_amd_ls_cfg_en )
> > +ssbd_amd_ls_cfg_set(true);
> > +
> > +return 0;
> > +
> > + ssbd_amd_ls_cfg_init_fail:
> > +for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> > +if ( ssbd_amd_smt_status[socket] != NULL )
> > + xfree(ssbd_amd_smt_status[socket]);
>
> There's no need for the if() here.
>
> > +setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
>
> The same feature must not first be forced, and the cleared. Please
> take a look at the implementation of the functions.
Will do.
> > +default_xen_ssbd_amd_ls_cfg_en = false;
> > +
> > +dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
> > +
> > +return 1;
>
> If the function returns 0 and 1 only, it looks like you've meant to
> use bool, false, and true respectively.
>
> Jan
>
Because it's more of an error code than boolean logic value. I can
switch it over to bool if you want.
Noted about the things I didn't directly reply to.
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
On Thu, Aug 09, 2018 at 02:42:13PM -0500, Brian Woods wrote: > @@ -237,8 +247,8 @@ static void __init print_details(enum ind_thunk thunk, > uint64_t caps) > - !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" : > - (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-", > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" : > + default_xen_ssbd_amd_ls_cfg_en? " SSBD+" : " SSBD-", This will change in the next version. I just noticed I only changed it in patch 2 and not 1. Sorry about that. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 0/2] SSBD AMD via LS CFG Enablement
This series of patches is for enabling SSBD via the LS_CFG MSR for family 15h-17h. The first patch make it so that the information is correctly displayed on boot. The last patch is for further enablement for Xen switching SSBD on or off internally, or for further control of SSBD for guests via the VIRT_SPEC_CTRL. Note: these should be applied after Jan's patch: x86/AMD: distinguish compute units from hyper-threads v1 -> v2: - changed some variable types - updated ssbd_amd_ls_cfg_set_nonsmt function - changed sbd_amd_ls_cfg_init to a presmp_initcall - deleted ssbd_amd_ls_cfg_set_init due to ^ and it happening later in the boot - various smaller cleanups Brian Woods (2): x86/spec-ctrl: add AMD SSBD LS_CFG in init print x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR xen/arch/x86/cpu/amd.c| 12 ++- xen/arch/x86/setup.c | 2 + xen/arch/x86/smpboot.c| 3 + xen/arch/x86/spec_ctrl.c | 202 +- xen/include/asm-x86/cpufeatures.h | 1 + xen/include/asm-x86/spec_ctrl.h | 5 + 6 files changed, 218 insertions(+), 7 deletions(-) -- 2.11.0 Brian Woods (2): x86/spec-ctrl: add AMD SSBD LS_CFG in init print x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR xen/arch/x86/cpu/amd.c| 12 +-- xen/arch/x86/smpboot.c| 3 + xen/arch/x86/spec_ctrl.c | 179 +- xen/include/asm-x86/cpufeatures.h | 1 + xen/include/asm-x86/spec_ctrl.h | 4 + 5 files changed, 192 insertions(+), 7 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
AMD CPUs. There needs to be locking logic for family 17h with SMT
enabled since both threads share the same MSR. Otherwise, a core just
needs to write to the LS_CFG MSR. For more information see:
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c | 7 +-
xen/arch/x86/smpboot.c | 3 +
xen/arch/x86/spec_ctrl.c| 174 +++-
xen/include/asm-x86/spec_ctrl.h | 2 +
4 files changed, 178 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 06c9e9661b..6a5fbcae22 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
ssbd_amd_ls_cfg_mask = 1ull << bit;
}
- if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
- if (opt_ssbd) {
- value |= ssbd_amd_ls_cfg_mask;
- wrmsr_safe(MSR_AMD64_LS_CFG, value);
- }
- }
/* MFENCE stops RDTSC speculation */
if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d4478e6132..af881ba30a 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -366,6 +366,9 @@ void start_secondary(void *unused)
if ( boot_cpu_has(X86_FEATURE_IBRSB) )
wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+if ( default_xen_ssbd_amd_ls_cfg_en )
+ssbd_amd_ls_cfg_set(true);
+
if ( xen_guest )
hypervisor_ap_setup();
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 62e6519d93..ff14b8f985 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
uint8_t __read_mostly default_xen_spec_ctrl;
uint8_t __read_mostly default_spec_ctrl_flags;
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 2
+struct ssbd_amd_ls_cfg_smt_status {
+spinlock_t lock;
+uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en = false;
+bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] =
{NULL};
static int __init parse_bti(const char *s)
{
@@ -237,8 +247,8 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_IBRS) ? "IBRS+" : "IBRS-",
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
- !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
- (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-",
+ !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" :
+ default_xen_ssbd_amd_ls_cfg_en? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "");
/*
@@ -487,6 +497,162 @@ static __init int parse_xpti(const char *s)
}
custom_param("xpti", parse_xpti);
+/*
+ * Enabling SSBD on AMD processers via the LS_CFG MSR
+ *
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just setting an MSR bit. For 17h, it depends
+ * if SMT is enabled. If SMT, are two threads that share a single MSR
+ * so there needs to be a lock and a virtual bit for each thread,
+ * otherwise it's the same as family 15h/16h.
+ */
+
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+uint64_t ls_cfg, new_ls_cfg;
+
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+
+if ( enable_ssbd )
+new_ls_cfg = ls_cfg | ssbd_amd_ls_cfg_mask;
+else
+new_ls_cfg = ls_cfg & ~ssbd_amd_ls_cfg_mask;
+
+if ( new_ls_cfg != ls_cfg )
+wrmsrl(MSR_AMD64_LS_CFG, new_ls_cfg);
+}
+
+static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
+{
+uint32_t socket, core, thread;
+uint64_t enable_mask;
+uint64_t ls_cfg;
+struct ssbd_amd_ls_cfg_smt_status *status;
+const struct cpuinfo_x86 *c = _cpu_data;
+
+socket = c->phys_proc_id;
+core = c->cpu_core_id;
+thread = c->apicid & (c->x86_num_siblings - 1);
+status = ssbd_amd_smt_status[socket] +
[Xen-devel] [PATCH v2 1/2] x86/spec-ctrl: add AMD SSBD LS_CFG in init print
Edit the initialization code for AMD's SSBD via the LS_CFG MSR and
enable it to pass the status to the initial spec-ctrl print_details at
boot.
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c| 13 ++---
xen/arch/x86/spec_ctrl.c | 9 +++--
xen/include/asm-x86/cpufeatures.h | 1 +
xen/include/asm-x86/spec_ctrl.h | 2 ++
4 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index bad5b43628..06c9e9661b 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -598,7 +598,7 @@ static void init_amd(struct cpuinfo_x86 *c)
* If the user has explicitly chosen to disable Memory Disambiguation
* to mitigiate Speculative Store Bypass, poke the appropriate MSR.
*/
- if (opt_ssbd) {
+ if (!ssbd_amd_ls_cfg_mask) {
int bit = -1;
switch (c->x86) {
@@ -607,8 +607,15 @@ static void init_amd(struct cpuinfo_x86 *c)
case 0x17: bit = 10; break;
}
- if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
- value |= 1ull << bit;
+ if (bit >= 0)
+ ssbd_amd_ls_cfg_mask = 1ull << bit;
+ }
+
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
+ setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
+ if (opt_ssbd) {
+ value |= ssbd_amd_ls_cfg_mask;
wrmsr_safe(MSR_AMD64_LS_CFG, value);
}
}
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 08e6784c4c..62e6519d93 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -50,6 +50,8 @@ bool __initdata bsp_delay_spec_ctrl;
uint8_t __read_mostly default_xen_spec_ctrl;
uint8_t __read_mostly default_spec_ctrl_flags;
+uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+
static int __init parse_bti(const char *s)
{
const char *ss;
@@ -210,10 +212,11 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
printk("Speculative mitigation facilities:\n");
/* Hardware features which pertain to speculative mitigations. */
-printk(" Hardware features:%s%s%s%s%s%s%s%s\n",
+printk(" Hardware features:%s%s%s%s%s%s%s%s%s\n",
(_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_SSBD)) ? " SSBD" : "",
+ boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? " SSBD" : "",
(e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "",
(caps & ARCH_CAPABILITIES_IBRS_ALL) ? " IBRS_ALL" : "",
(caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : "",
@@ -225,7 +228,7 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
printk(" Compiled-in support: INDIRECT_THUNK\n");
/* Settings for Xen's protection, irrespective of guests. */
-printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s\n",
+printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE? "LFENCE" :
@@ -234,6 +237,8 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_IBRS) ? "IBRS+" : "IBRS-",
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
+ !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
+ (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "");
/*
diff --git a/xen/include/asm-x86/cpufeatures.h
b/xen/include/asm-x86/cpufeatures.h
index b90aa2d046..9383d4058b 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV, (FSCAPINTS+0)*32+18) /* RSB
overwrite needed for
XEN_CPUFEATURE(SC_RSB_HVM, (FSCAPINTS+0)*32+19) /* RSB overwrite needed
for HVM */
XEN_CPUFEATURE(NO_XPTI, (FSCAPINTS+0)*32+20) /* XPTI mitigation not in
use */
XEN_CPUFEATURE(SC_MSR_IDLE, (FSCAPINTS+0)*32+21) /* (SC_MSR_PV ||
SC_MSR_HVM) && default_xen_spec_ctr
Re: [Xen-devel] [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR
On Tue, Aug 07, 2018 at 01:51:07AM -0600, Jan Beulich wrote: > >>> On 06.08.18 at 21:07, wrote: > > On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote: > >> >>> On 02.08.18 at 00:20, wrote: > >> > On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote: > >> >> Code structure wise this looks to undo a fair part of what patch > >> >> 1 has done. It would be nice to limit code churn. > >> > > >> > Patch 1 stand alone just to improve reporting the capabilities of the > >> > processor. Currently Xen doesn't mention anything if SSBD is actually > >> > enable on AMD processors. Patch 2-3 add it where Xen can it > >> > dynamically. > >> > >> Which is all fine, but couldn't patch 1 be written in a way that the > >> next one(s) don't have to turn code structure all over again. > > > > Rather than change 1, I changed patch 3 (well now patch 2). > > > >> >> Why can't this be called from init_speculation_mitigations()? > >> > > >> > IIRC I was having memory init problems with. It was moved to later in > >> > the boot so that xmalloc would work correctly. I haven't touched this > >> > code in over month. If you want a 100% tested answer I can retest > >> > putting it in init_speculation_mitigations(). > >> > >> Would be nice if that could be arranged for, as the rather specialized > >> call looks pretty odd in (iirc __start_xen(); iirc because you've dropped > >> too much of the context in your reply, and I'm too lazy to dig out the > >> original patch again). > > > > It was __start_xen(). Well, IIRC xalloc didn't work (well writing to > > the address given) until after arch_init_memory(). For it to work in > > init_speculation_mitigations(), I'd assume you'd need to call > > arch_init_memory() before init_speculation_mitigations(). > > I don't think that's a viable option, but it could certainly be explored. > I wonder though whether you can't get away without allocation at > this early point. Well, the thing is that you could use DECLARE_PER_CPU but then you have to initialize it during each cpu start up, but two logical cpus share a lock and only one needs to touch it etc. I found it better to just initialize everything on the boot cpu before others are brought up, but the whole thing is a little messy. (See the last comment.) > >> >> Still I wonder whether less code duplication wouldn't be better. > >> > > >> > current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called. > >> > >> By "isn't available" you mean "hasn't been populated yet"? Else > >> I don't understand. > > > > It hasn't been populated yet. > > Not even boot_cpu_data? boot_cpu_data is, but current_cpu_data and nr_sockets is not. > >> >> I find such a hard-coded upper bound quite concerning. Is nr_sockets > >> >> not correct in the AMD case? If so, that would want fixing, such that > >> >> you can use the variable here. > >> > > >> > It's been a while since I wrote this but IIRC, it has to do with > >> > nr_sockets either being off or not available when the code is run. > >> > For Family 17h which the patches are for, there's a max of two sockets. > >> > >> I've implied that from how you wrote the patches, but such hard coding > >> will only make for more code churn later on. Try to be as generic as > >> possible. > > > > Well, nr_sockets gets set in smp_prepare_cpus, which gets called after > > init_speculation_mitigations() and ssbd_amd_ls_cfg_init(). It could be > > put later on in the boot, but it needs to be run at least before other > > cpus are brought online. I'm welcome to any suggestions about how to > > restructure things though. > > Did you consider using a presmp-initcall? > > Jan I've been looking at it, seems like that could work. You'd still need something in start_secondary but it would take the init call out of __start_xen(). I'll test that. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR
t;> Btw - why the xen_ prefix for the variable? > > > > See the first reply, but basically it's for if Xen has SSBD turned on > > or not via using the LS_CFG MSR. > > Personally I'd prefer if the xen_-prefixed sub-name-space was left to > the public interface. Make it an infix if you want to express what you've > explained? > > Jan Noted. I'll change that. Thanks again for the feedback. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR
On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote: > Code structure wise this looks to undo a fair part of what patch > 1 has done. It would be nice to limit code churn. Patch 1 stand alone just to improve reporting the capabilities of the processor. Currently Xen doesn't mention anything if SSBD is actually enable on AMD processors. Patch 2-3 add it where Xen can it dynamically. > Why can't this be called from init_speculation_mitigations()? IIRC I was having memory init problems with. It was moved to later in the boot so that xmalloc would work correctly. I haven't touched this code in over month. If you want a 100% tested answer I can retest putting it in init_speculation_mitigations(). > Please be consistent with types: ssbd_amd_ls_cfg_mask is > uint64_t, in which case variables like the one here should be too. I think that was left over from something in init_amd, but yes. > Missing blanks. Noted > Please simplify this to have just a single rdmsrl() and wrmsrl() > each in the function. Will do. > You really should notice such anomalies / inconsistencies yourself: > You properly use uint64_t here, so why not also uint32_t on the > preceding line? That said - why a fixed width type anyway for > those variables - unsigned int looks to be just fine there. IIRC they're __32 in struct I'm reading from so I decided to use that. I can change them though, that's easily. > This function is called from exactly one place, with the > parameter set to true. Makes me wonder what the actual > purpose of this patch is. See earlier in this email. > Still I wonder whether less code duplication wouldn't be better. current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called. > This hides very well an assumption of there only ever being two > threads. Please don't. I'm also having a hard time seeing why > c->apicid being (non-)zero matters. Or wait - do you mean & > instead of && above (then also in ssbd_amd_ls_cfg_set_smt())? Yes... That was meant to be a &. Thanks for catching that. > Most noticeable here, but applicable elsewhere: The canonical > placement is > > void __init ssbd_amd_ls_cfg_init(void) > unsigned int please for anything that can't go negative. And a > blank is missing after the comma here, while there's one too > many on the line before. > > You also don't look to be altering the data c points to - please > make this a pointer to const (and check whether there are > other places wanting such a transformation). > Blank lines between case blocks please. Noted about the above. > I find such a hard-coded upper bound quite concerning. Is nr_sockets > not correct in the AMD case? If so, that would want fixing, such that > you can use the variable here. It's been a while since I wrote this but IIRC, it has to do with nr_sockets either being off or not available when the code is run. For Family 17h which the patches are for, there's a max of two sockets. > Style: Blank before * and no blank before (. > Perhaps better > spin_lock_init(_amd_smt_status[socket][core].lock); > ssbd_amd_smt_status[socket][core].mask = 0; > ? > Labels indented by at least one blank please. Noted about the above. > Btw - why the xen_ prefix for the variable? See the first reply, but basically it's for if Xen has SSBD turned on or not via using the LS_CFG MSR. > Pointless "return" at end of function. > > Jan Noted. Thanks for all the feedback. I'll try and get v2 out in a couple of days or so. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/3] x86/spec-ctrl: add AMD SSBD LS_CFG in init print
On Tue, Jul 31, 2018 at 04:47:48AM -0600, Jan Beulich wrote: > Pointless initializer. Noted > Why log the same string twice? Simply OR together both conditions. > Also please don't omit the blank before the ? operator. Both remarks > apply as well further down. Because they're completely different implementation at this point. I thought about naming it something else but couldn't think of something. It also reads easier with having them split up where there isn't a ton of logic in a single ternary operator. There can be easily changed. > I've peeked ahead into the following patches, and I can't see > why this needs to be a synthetic feature flag. We use such flags > for the purpose of alternative instruction patching, but you don't > do anything like that. > > Jan I was talking Andy earily about a SSBD (via VIRT_SPEC_CTRL) implementation and he mentioned something about ALTERNATIVE C funcs which would allow to allow HVM guests to be able control SSBD. In that case having a synthetic feature flag is useful for helping to differiate between SSBD via the LS_CFG MSR vs SPEC_CTRL in the future. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/spec-ctrl: Add defines and variables for AMD SSBD support
On Tue, Jul 31, 2018 at 04:44:57AM -0600, Jan Beulich wrote: > Whether these additions fit the purpose can only be told when > seeing their use. Please fold this into the patch using these. > > Jan I was trying to split up patch 3 but I'll combine 2 and 3. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Build Inconsistencies
On Wed, Jul 25, 2018 at 10:27:06AM +0200, Roger Pau Monné wrote: > I'm afraid I'm not able to find the error in the logs. I've tried > grepping for 'error:' or '***', but I haven't been able to find > anything. > > Do you happen to have the exact cause of the build failure? > > > AFAICT a `make debball` doesn't call the clean targets, so you are > doing something else here apart from a `make debball`. > > Thanks, Roger. I just looked and it I accident did two make cleans rather than a clean and build in the j120 log. It wasn't producing a deb though, after doing cleans and distcleans it still wasn't producing a new debball. I'm guessing it was transient because when I tried to reproduce it again today, it was successful, repeatedly. Either way, sorry about this. Next time this happens I'll make sure to pull a fresh copy to double check rather than just a distclean. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/3] SSBD AMD via LS CFG Enablement
This series of patches is for enabling SSBD via the LS_CFG MSR for family 15h-17h. The first patch make it so that the information is correctly displayed on boot. The last two patches are for further enablement for Xen switching SSBD on or off internally, or for further control of SSBD for guests via the VIRT_SPEC_CTRL. Note: these should be applied after Jan's patch: x86/AMD: distinguish compute units from hyper-threads Brian Woods (3): x86/spec-ctrl: add AMD SSBD LS_CFG in init print x86/spec-ctrl: Add defines and variables for AMD SSBD support x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR xen/arch/x86/cpu/amd.c| 7 +- xen/arch/x86/setup.c | 2 + xen/arch/x86/smpboot.c| 3 + xen/arch/x86/spec_ctrl.c | 218 +- xen/include/asm-x86/cpufeatures.h | 1 + xen/include/asm-x86/spec_ctrl.h | 5 + 6 files changed, 231 insertions(+), 5 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/3] x86/spec-ctrl: Add defines and variables for AMD SSBD support
In preparation for adding switchable SSBD, add some defines and
variables.
Signed-off-by: Brian Woods
---
xen/arch/x86/spec_ctrl.c| 10 ++
xen/include/asm-x86/spec_ctrl.h | 3 +++
2 files changed, 13 insertions(+)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 62e6519d93..baef907322 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
uint8_t __read_mostly default_xen_spec_ctrl;
uint8_t __read_mostly default_spec_ctrl_flags;
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 2
+struct ssbd_amd_ls_cfg_smt_status {
+spinlock_t lock;
+uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en = false;
+bool __read_mostly xen_ssbd_amd_ls_cfg_en = false;
uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] =
{NULL};
static int __init parse_bti(const char *s)
{
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 6aebfa9e4f..c47647771a 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -39,6 +39,9 @@ extern uint8_t opt_xpti;
#define OPT_XPTI_DOMU 0x02
extern uint64_t ssbd_amd_ls_cfg_mask;
+extern bool xen_ssbd_amd_ls_cfg_en;
+extern void ssbd_amd_ls_cfg_init(void);
+extern void ssbd_amd_ls_cfg_set(bool enable_ssbd);
static inline void init_shadow_spec_ctrl_state(void)
{
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/3] x86/spec-ctrl: add AMD SSBD LS_CFG in init print
Edit the initialization code for AMD's SSBD via the LS_CFG MSR and
enable it to pass the status to the initial spec-ctrl print_details at
boot.
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c| 13 ++---
xen/arch/x86/spec_ctrl.c | 9 +++--
xen/include/asm-x86/cpufeatures.h | 1 +
xen/include/asm-x86/spec_ctrl.h | 2 ++
4 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index bad5b43628..06c9e9661b 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -598,7 +598,7 @@ static void init_amd(struct cpuinfo_x86 *c)
* If the user has explicitly chosen to disable Memory Disambiguation
* to mitigiate Speculative Store Bypass, poke the appropriate MSR.
*/
- if (opt_ssbd) {
+ if (!ssbd_amd_ls_cfg_mask) {
int bit = -1;
switch (c->x86) {
@@ -607,8 +607,15 @@ static void init_amd(struct cpuinfo_x86 *c)
case 0x17: bit = 10; break;
}
- if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
- value |= 1ull << bit;
+ if (bit >= 0)
+ ssbd_amd_ls_cfg_mask = 1ull << bit;
+ }
+
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
+ setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
+ if (opt_ssbd) {
+ value |= ssbd_amd_ls_cfg_mask;
wrmsr_safe(MSR_AMD64_LS_CFG, value);
}
}
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 08e6784c4c..62e6519d93 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -50,6 +50,8 @@ bool __initdata bsp_delay_spec_ctrl;
uint8_t __read_mostly default_xen_spec_ctrl;
uint8_t __read_mostly default_spec_ctrl_flags;
+uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+
static int __init parse_bti(const char *s)
{
const char *ss;
@@ -210,10 +212,11 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
printk("Speculative mitigation facilities:\n");
/* Hardware features which pertain to speculative mitigations. */
-printk(" Hardware features:%s%s%s%s%s%s%s%s\n",
+printk(" Hardware features:%s%s%s%s%s%s%s%s%s\n",
(_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "",
(_7d0 & cpufeat_mask(X86_FEATURE_SSBD)) ? " SSBD" : "",
+ boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? " SSBD" : "",
(e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "",
(caps & ARCH_CAPABILITIES_IBRS_ALL) ? " IBRS_ALL" : "",
(caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : "",
@@ -225,7 +228,7 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
printk(" Compiled-in support: INDIRECT_THUNK\n");
/* Settings for Xen's protection, irrespective of guests. */
-printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s\n",
+printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE? "LFENCE" :
@@ -234,6 +237,8 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_IBRS) ? "IBRS+" : "IBRS-",
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
+ !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
+ (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "");
/*
diff --git a/xen/include/asm-x86/cpufeatures.h
b/xen/include/asm-x86/cpufeatures.h
index b90aa2d046..9383d4058b 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV, (FSCAPINTS+0)*32+18) /* RSB
overwrite needed for
XEN_CPUFEATURE(SC_RSB_HVM, (FSCAPINTS+0)*32+19) /* RSB overwrite needed
for HVM */
XEN_CPUFEATURE(NO_XPTI, (FSCAPINTS+0)*32+20) /* XPTI mitigation not in
use */
XEN_CPUFEATURE(SC_MSR_IDLE, (FSCAPINTS+0)*32+21) /* (SC_MSR_PV ||
SC_MSR_HVM) && default_xen_spec_ctr
[Xen-devel] [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR
Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
AMD CPUs. There needs to be locking logic for family 17h with SMT
enabled since both threads share the same MSR. Otherwise, a core just
needs to write to the LS_CFG MSR. For more information see:
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c | 10 +--
xen/arch/x86/setup.c | 2 +
xen/arch/x86/smpboot.c | 3 +
xen/arch/x86/spec_ctrl.c | 201 ++-
4 files changed, 207 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 06c9e9661b..e7ec0d99a7 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -607,16 +607,10 @@ static void init_amd(struct cpuinfo_x86 *c)
case 0x17: bit = 10; break;
}
- if (bit >= 0)
- ssbd_amd_ls_cfg_mask = 1ull << bit;
- }
- if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
- if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
+ if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ ssbd_amd_ls_cfg_mask = 1ull << bit;
setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
- if (opt_ssbd) {
- value |= ssbd_amd_ls_cfg_mask;
- wrmsr_safe(MSR_AMD64_LS_CFG, value);
}
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 419b46c033..b551852cbd 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1579,6 +1579,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
arch_init_memory();
+ssbd_amd_ls_cfg_init();
+
alternative_instructions();
local_irq_enable();
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d4478e6132..07760c920d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -366,6 +366,9 @@ void start_secondary(void *unused)
if ( boot_cpu_has(X86_FEATURE_IBRSB) )
wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+if ( xen_ssbd_amd_ls_cfg_en )
+ssbd_amd_ls_cfg_set(true);
+
if ( xen_guest )
hypervisor_ap_setup();
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index baef907322..006e8fb14b 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -248,7 +248,7 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
- (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-",
+ xen_ssbd_amd_ls_cfg_en? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "");
/*
@@ -497,6 +497,201 @@ static __init int parse_xpti(const char *s)
}
custom_param("xpti", parse_xpti);
+/*
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just need to set an MSR bit. For 17h, it
+ * depends if SMT is enabled. If SMT, are two threads share a single
+ * MSR so there needs to be a lock and a virtual bit for each thread.
+ */
+
+/* used for non SMT mitigations (no shared MSRs) */
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+unsigned long ls_cfg;
+
+if ( enable_ssbd )
+{
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+if ( !(ls_cfg & ssbd_amd_ls_cfg_mask) )
+{
+ls_cfg |= ssbd_amd_ls_cfg_mask;
+wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+}
+}
+else
+{
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+if (ls_cfg & ssbd_amd_ls_cfg_mask)
+{
+ls_cfg &= ~ssbd_amd_ls_cfg_mask;
+wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+}
+}
+}
+
+/* used for family 17h with SMT enabled (shared MSRs) */
+static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
+{
+__u32 socket, core, thread;
+uint64_t enable_mask;
+unsigned long ls_cfg;
+struct ssbd_amd_ls_cfg_smt_status *status;
+struct cpuinfo_x86 *c = _cpu_data;
+
+socket = c->phys_proc_id;
+core = c->cpu_core_id;
+thread = c->apicid && (c->x86_num_siblings - 1);
+
+status = ssbd_amd_smt_status[socket] + core;
+enable_mask = (1ull << thread);
+spin_lock(>lock);
+
+if ( enable_ssbd )
+{
+if ( !(status->mask & enable_mask) )
+{
+status->mask |= enable_mask;
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+if ( !(ls_cfg &
Re: [Xen-devel] [PATCH 4/8] x86/AMD: distinguish compute units from hyper-threads
On Wed, Jul 11, 2018 at 06:07:42AM -0600, Jan Beulich wrote:
> Fam17 replaces CUs by HTs, which we should reflect accordingly, even if
> the difference is not very big. The most relevant change (requiring some
> code restructuring) is that the topoext feature no longer means there is
> a valid CU ID.
>
> Take the opportunity and convert wrongly plain int variables in
> set_cpu_sibling_map() to unsigned int.
>
> Signed-off-by: Jan Beulich
>
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -504,17 +504,23 @@ static void amd_get_topology(struct cpui
> u32 eax, ebx, ecx, edx;
>
> cpuid(0x801e, , , , );
> -c->compute_unit_id = ebx & 0xFF;
> c->x86_num_siblings = ((ebx >> 8) & 0x3) + 1;
> +
> + if (c->x86 < 0x17)
> + c->compute_unit_id = ebx & 0xFF;
> + else {
> + c->cpu_core_id = ebx & 0xFF;
> + c->x86_max_cores /= c->x86_num_siblings;
> + }
> }
>
> if (opt_cpu_info)
> printk("CPU %d(%d) -> Processor %d, %s %d\n",
> cpu, c->x86_max_cores, c->phys_proc_id,
> - cpu_has(c, X86_FEATURE_TOPOEXT) ? "Compute Unit" :
> - "Core",
> - cpu_has(c, X86_FEATURE_TOPOEXT) ? c->compute_unit_id :
> - c->cpu_core_id);
> + c->compute_unit_id != INVALID_CUID ? "Compute Unit"
> + : "Core",
> + c->compute_unit_id != INVALID_CUID ?
> c->compute_unit_id
> + : c->cpu_core_id);
> }
>
> static void early_init_amd(struct cpuinfo_x86 *c)
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -236,33 +236,41 @@ static void link_thread_siblings(int cpu
> cpumask_set_cpu(cpu2, per_cpu(cpu_core_mask, cpu1));
> }
>
> -static void set_cpu_sibling_map(int cpu)
> +static void set_cpu_sibling_map(unsigned int cpu)
> {
> -int i;
> +unsigned int i;
> struct cpuinfo_x86 *c = cpu_data;
>
> cpumask_set_cpu(cpu, _sibling_setup_map);
>
> cpumask_set_cpu(cpu, socket_cpumask[cpu_to_socket(cpu)]);
> +cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
> +cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>
> if ( c[cpu].x86_num_siblings > 1 )
> {
> for_each_cpu ( i, _sibling_setup_map )
> {
> -if ( cpu_has(c, X86_FEATURE_TOPOEXT) ) {
> -if ( (c[cpu].phys_proc_id == c[i].phys_proc_id) &&
> - (c[cpu].compute_unit_id == c[i].compute_unit_id) )
> +if ( cpu == i || c[cpu].phys_proc_id != c[i].phys_proc_id )
> +continue;
> +if ( c[cpu].compute_unit_id != INVALID_CUID &&
> + c[i].compute_unit_id != INVALID_CUID )
> +{
> +if ( c[cpu].compute_unit_id == c[i].compute_unit_id )
> link_thread_siblings(cpu, i);
> -} else if ( (c[cpu].phys_proc_id == c[i].phys_proc_id) &&
> -(c[cpu].cpu_core_id == c[i].cpu_core_id) ) {
> -link_thread_siblings(cpu, i);
> }
> +else if ( c[cpu].cpu_core_id != XEN_INVALID_CORE_ID &&
> + c[i].cpu_core_id != XEN_INVALID_CORE_ID )
> +{
> +if ( c[cpu].cpu_core_id == c[i].cpu_core_id )
> +link_thread_siblings(cpu, i);
> +}
> +else
> +printk(XENLOG_WARNING
> + "CPU%u: unclear relationship with CPU%u\n",
> + cpu, i);
> }
> }
> -else
> -{
> -cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> -}
>
> if ( c[cpu].x86_max_cores == 1 )
> {
>
>
>
>
Side note: if cpu_core_id isn't the logical cpu, it might be worth
updating the comments in processor.h
Reviewed-by: Brian Woods
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Dom0 Failing to Boot with Recent Linux Kernels (Spectre Mitigations)
On Thu, Jun 21, 2018 at 01:55:54AM +0800, Andrew Cooper wrote: > That is Linux trying to enable the SSBD via the native mechanism (on > Fam17h hardware I'm guessing, give the bit position). > > Like many of the other mitigations, a PV guest knows it is virtualised > and shouldn’t be playing with this MSR. > > OTOH, Xen should be implementing the AMD SSBD spec and allowing for > virtualised control of SSBD. Sadly, I was only show the whitepaper with > insufficient time before the SSBD embargo to implement it in XSA-263, > and haven't had time since. > > ~Andrew I'll take a look into updating it for AMD related processors then. I will talk with some people internally who did the work for Linux and see what needs to be done etc. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Dom0 Failing to Boot with Recent Linux Kernels (Spectre Mitigations)
On Wed, Jun 20, 2018 at 12:20:21PM -0500, Brian Woods wrote: > On Wed, Jun 20, 2018 at 08:34:13AM +0200, Juergen Gross wrote: > Juergen, > > It works for Dom0. You see a lot of messages that are like: > (XEN) emul-priv-op.c:1166:d0v1 Domain attempted WRMSR c0011020 from > 0x02068000 to 0x02068400 > (XEN) emul-priv-op.c:1166:d0v0 Domain attempted WRMSR c0011020 from > 0x02068000 to 0x02068400 > > But it boots up. I'll test a DomU with a newer kernel after I get back > from lunch. > > -- > Brian Woods It works on PV and HVM guests. I'll reply to Andy's email soon. Have a meeting in 2 minutes... -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Dom0 Failing to Boot with Recent Linux Kernels (Spectre Mitigations)
On Wed, Jun 20, 2018 at 08:34:13AM +0200, Juergen Gross wrote: > Brian, would you please give the attached patch a try? > > > Juergen Juergen, It works for Dom0. You see a lot of messages that are like: (XEN) emul-priv-op.c:1166:d0v1 Domain attempted WRMSR c0011020 from 0x02068000 to 0x02068400 (XEN) emul-priv-op.c:1166:d0v0 Domain attempted WRMSR c0011020 from 0x02068000 to 0x02068400 But it boots up. I'll test a DomU with a newer kernel after I get back from lunch. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Dom0 Failing to Boot with Recent Linux Kernels (Spectre Mitigations)
I'm currently seeing an issue where when booting from a recent Linux kernel without nospec_store_bypass_disable. There's a NULL pointer having to do with a lock. I put some printks in and it seems that in arch/x86/kernel/process.c that speculative_store_bypass_ht_init isn't getting called which initializes the spin lock. Here's the serial output: [7.748191] BUG: unable to handle kernel NULL pointer dereference at 0008 [7.748202] PGD 0 P4D 0 [7.748208] Oops: 0002 [#1] SMP NOPTI [7.748212] Modules linked in: ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 fscrypto raid10 raid456 async_raid6_recov async_memcpy async_pq async_xore [7.748261] CPU: 4 PID: 321 Comm: (journald) Not tainted 4.17.2+ #1 [7.748266] Hardware name: AMD Corporation Diesel Debug/Diesel Debug, BIOS TDD1007E 04/16/2018 [7.748277] RIP: e030:_raw_spin_lock+0xc/0x20 [7.748293] RSP: e02b:c9004709feb8 EFLAGS: 00010046 [7.748297] RAX: RBX: 880285715b30 RCX: ea0009ce30df [7.748302] RDX: 0001 RSI: 0008 RDI: 0008 [7.748308] RBP: 0400 R08: R09: 0007 [7.748313] R10: 0040 R11: 88027a9bc800 R12: 02068000 [7.748318] R13: R14: 880273aa0080 R15: [7.748331] FS: 7fc72e830940() GS:88028570() knlGS: [7.748336] CS: e033 DS: ES: CR0: 80050033 [7.748341] CR2: 0008 CR3: 000278c6c000 CR4: 00040660 [7.748347] Call Trace: [7.748354] speculative_store_bypass_update+0x72/0x160 [7.748361] ssb_prctl_set+0x67/0xb0 [7.748367] do_seccomp+0x477/0x6c0 [7.748385] do_syscall_64+0x55/0x100 [7.748390] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [7.748395] RIP: 0033:0x7fc72ce13229 [7.748399] RSP: 002b:7ffda5166968 EFLAGS: 0246 ORIG_RAX: 013d [7.748405] RAX: ffda RBX: 55745ea1dfe0 RCX: 7fc72ce13229 [7.748410] RDX: 55745ea1dfe0 RSI: RDI: 0001 [7.748415] RBP: 55745ea5b740 R08: 55745ea1dfe0 R09: 403e [7.748421] R10: 000d R11: 0246 R12: 7ffda51669c0 [7.748426] R13: 7ffda51669b8 R14: 7fc72e560c14 R15: 002a [7.748431] Code: ff 01 00 00 75 05 48 89 d8 5b c3 e8 1f 8f 9f ff 48 89 d8 5b c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 31 c0 ba 01 00 00 00 0f [7.748483] RIP: _raw_spin_lock+0xc/0x20 RSP: c9004709feb8 [7.748487] CR2: 0008 [7.748492] ---[ end trace cf886bf535fde244 ]--- With nospec_store_bypass_disable, it boots fine etc. It seems to works fine (at least Dom0 can boot). Linux Kernel -> 4.17.2 Xen -> current HEAD on master Is this a known or expected problem? -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 for-4.11] x86/pv: Hide more EFER bits from PV guests
On Mon, May 07, 2018 at 11:00:23AM +0100, Andrew Cooper wrote:
> We don't advertise SVM in CPUID so a PV guest shouldn't be under the
> impression that it can use SVM functionality, but despite this, it really
> shouldn't see SVME set when reading EFER.
>
> On Intel processors, 32bit PV guests don't see, and can't use SYSCALL.
>
> Introduce EFER_KNOWN_MASK to whitelist the features Xen knows about, and use
> this to clamp the guests view.
>
> Take the opportunity to reuse the mask to simplify svm_vmcb_isvalid(), and
> change "undefined" to "unknown" in the print message, as there is at least
> EFER.TCE (Translation Cache Extension) defined but unknown to Xen.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Release-acked-by: Juergen Gross <jgr...@suse.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
> CC: Brian Woods <brian.wo...@amd.com>
>
> Arguably, this wants backporting to the stable trees, so should be considered
> for 4.11 at this point.
> ---
> xen/arch/x86/hvm/svm/svmdebug.c | 5 ++---
> xen/arch/x86/pv/emul-priv-op.c | 11 +--
> xen/include/asm-x86/msr-index.h | 3 +++
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
> index 6c215d1..d35e405 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -133,9 +133,8 @@ bool svm_vmcb_isvalid(const char *from, const struct
> vmcb_struct *vmcb,
> PRINTF("DR7: bits [63:32] are not zero (%#"PRIx64")\n",
> vmcb_get_dr7(vmcb));
>
> -if ( efer & ~(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | EFER_SVME |
> - EFER_LMSLE | EFER_FFXSE) )
> -PRINTF("EFER: undefined bits are not zero (%#"PRIx64")\n", efer);
> +if ( efer & ~EFER_KNOWN_MASK )
> +PRINTF("EFER: unknown bits are not zero (%#"PRIx64")\n", efer);
>
> if ( hvm_efer_valid(v, efer, -1) )
> PRINTF("EFER: %s (%"PRIx64")\n", hvm_efer_valid(v, efer, -1), efer);
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index 15f42b3..ce2ec76 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -867,9 +867,16 @@ static int read_msr(unsigned int reg, uint64_t *val,
> return X86EMUL_OKAY;
>
> case MSR_EFER:
> -*val = read_efer();
> +/* Hide unknown bits, and unconditionally hide SVME from guests. */
> +*val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
> +/*
> + * Hide the 64-bit features from 32-bit guests. SCE has
> + * vendor-dependent behaviour.
> + */
> if ( is_pv_32bit_domain(currd) )
> -*val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
> +*val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE |
> + (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL
> + ? EFER_SCE : 0));
> return X86EMUL_OKAY;
>
> case MSR_K7_FID_VID_CTL:
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index c9f44eb..6d94d65 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -31,6 +31,9 @@
> #define EFER_LMSLE (1<<_EFER_LMSLE)
> #define EFER_FFXSE (1<<_EFER_FFXSE)
>
> +#define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA |
> EFER_NX | \
> + EFER_SVME | EFER_LMSLE | EFER_FFXSE)
> +
> /* Speculation Controls. */
> #define MSR_SPEC_CTRL0x0048
> #define SPEC_CTRL_IBRS (_AC(1, ULL) << 0)
> --
> 2.1.4
>
Reviewed-by: Brian Woods <brian.wo...@amd.com>
--
Brian Woods
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] Add Brian Woods as Designated reviewer to AMD IOMMU and AMD SVM
On Tue, Apr 24, 2018 at 04:56:23PM +0100, Lars Kurth wrote: > This was discussed in an IRC discussion post the April x86 meeting. > > Signed-off-by: Lars Kurth <lars.ku...@citrix.com> > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index cc1fdc013f..fab76b0af4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -144,12 +144,14 @@ F: tools/libacpi/ > > AMD IOMMU > M: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com> > +R: Brian Woods <brian.wo...@amd.com> > S: Maintained > F: xen/drivers/passthrough/amd/ > > AMD SVM > M: Boris Ostrovsky <boris.ostrov...@oracle.com> > M: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com> > +R: Brian Woods <brian.wo...@amd.com> > S: Supported > F: xen/arch/x86/hvm/svm/ > F: xen/arch/x86/cpu/vpmu_amd.c > -- > 2.13.0 > Acked-By: Brian Woods <brian.wo...@amd.com> -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 0/8] Introduce AMD SVM AVIC
On Tue, Apr 03, 2018 at 06:01:16PM -0500, Janakarajan Natarajan wrote: > OVERVIEW > > This patchset is the first of a two-part patch series to introduce > the AMD Advanced Virtual Interrupt Controller (AVIC) support. > > The AVIC hardware virtualizes local APIC registers of each vCPU via > the virtual APIC (vAPIC) backing page. This allows the guest to access > certain APIC registers without the need for emulation of hardware > behaviour in the hypervisor. More information about AVIC can be found in > > * AMD64 Architecture Programmers Manual Volume 2 - System Programming > https://support.amd.com/TechDocs/24593.pdf > > For SVM AVIC, we extend the existing SVM driver to: > * Check CPUID to detect AVIC support in the processor. > * Program new fields in VMCB to enable AVIC. > * Introduce new AVIC data structures and add code to manage them. > * Handle two new AVIC #VMEXITs. > * Add new interrupt injection code using vAPIC backing page > instead of the existing V_IRQ, V_INTR_PRIO, V_INTR_VECTOR and > V_IGN_TPR fields. > > This patchset does not enable AVIC by default since it does not > yet support nested VMs. Until then, users can enable SVM AVIC by > specifying Xen parameter svm=avic. > > Later, in Part 2, we will introduce the IOMMU AVIC support, which > provides speed-up for the PCI device passthrough use case by allowing > the IOMMU hardware to inject interrupts directly into the guest via > the vAPIC backing page. > > OVERALL PERFORMANCE > === > AVIC is available on AMD Family 15h models 6Xh (Carrizo) processors > and newer. An AMD Family 17h Epyc processor is used to collect the > performance data shown below. > > Generally, SVM AVIC alone (w/o IOMMU AVIC) should provide overall > speed-up for HVM guest since it does not require #VMEXIT into the > hypervisor to emulate certain guest accesses to local APIC registers. > > It should also improve performance when the hypervisor wants to > inject interrupts into a running vcpu. It can do this by setting the > corresponding IRR bit in the vAPIC backing page and triggering the > AVIC_DOORBELL. > > For sending IPI interrupts between running vcpus in a Linux guest, > Xen defaults to using event channels. However, in case of > non-paravirtualized guests, AVIC can also provide performance > improvements for sending IPIs. > > BENCHMARK: HACKBENCH > > For measuring IPI performance used for scheduling workload, some > performance numbers are collected using hackbench. > > # hackbench -p -l 10 > Running in process mode with 10 groups using 40 file descriptors > each (== 400 tasks) > Each sender will pass 10 messages of 100 bytes > | 3 vcpus (sec)| > -- > No AVIC w/ xen_nopv | 517 | > AVIC w/ xen_nopv | 173 | > No AVIC w/o xen_nopv | 141 | > AVIC w/o xen_nopv | 135 | > > Each benchmark test was averaged over 10 runs. > > CURRENT UNTESTED USE_CASES > == > * Nested VM > > Any feedback and comments are very much appreciated. > > Suravee Suthikulpanit (8): > x86/SVM: Modify VMCB fields to add AVIC support > x86/HVM/SVM: Add AVIC initialization code > x86/SVM: Add AVIC vmexit handlers > x86/SVM: Add vcpu scheduling support for AVIC > x86/SVM: Add interrupt management code via AVIC > x86/HVM: Hook up miscellaneous AVIC functions > x86/SVM: Introduce svm command line option > x86/SVM: Add AMD AVIC key handler > > docs/misc/xen-command-line.markdown | 16 + > xen/arch/x86/hvm/svm/Makefile | 1 + > xen/arch/x86/hvm/svm/avic.c | 626 > > xen/arch/x86/hvm/svm/intr.c | 4 + > xen/arch/x86/hvm/svm/svm.c | 77 - > xen/arch/x86/hvm/svm/vmcb.c | 3 + > xen/arch/x86/hvm/vlapic.c | 20 +- > xen/arch/x86/hvm/vmx/vmx.c | 8 +- > xen/include/asm-x86/hvm/hvm.h | 4 +- > xen/include/asm-x86/hvm/svm/avic.h | 43 +++ > xen/include/asm-x86/hvm/svm/svm.h | 2 + > xen/include/asm-x86/hvm/svm/vmcb.h | 52 ++- > xen/include/asm-x86/hvm/vlapic.h| 4 + > xen/include/asm-x86/msr-index.h | 1 + > 14 files changed, 831 insertions(+), 30 deletions(-) > create mode 100644 xen/arch/x86/hvm/svm/avic.c > create mode 100644 xen/include/asm-x86/hvm/svm/avic.h > > -- > 2.11.0 > Not a Xen expert by any means but I've looked over the patch set and nothing jumps out as wrong. Reviewed-by: Brian Woods <brian.wo...@amd.com> -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Setting up a Xen x86 community call
On Fri, Mar 02, 2018 at 04:39:59PM +0100, Lars Kurth wrote: > Hi all, > (sorry for the extensive distribution list - I went through MAINTAINERS and > people who may have an interest) > > I would like to start organizing a recurring x86 community call to discuss > and sync-up on upcoming features for Xen on x86. This call would mirror and > follow a similar structure to the ARM call (see > http://xen.markmail.org/thread/xqdxvqcjpf2y5ftu for the last one) > > I expect that the call will contain > > a) Coordination and Planning > Coordinating who does what, what needs attention, what is blocked, etc. > I would prepare a list of non-merged patch series of a certain size (e.g. > more than 5 patches) and attach to the invite > If anything is missed, I would expect that these are sent to me before the > meeting > > b) Design and architecture related discussions: in particular for bigger, > more complex items, ... > Although all of this could be done by email, in reality, we are all human and > many people find it easier to collaborate > and communicate by talking to each other, rather than by email. This is not a > must, but an option to highlight issues > > c) Demos, Sharing of Experiences, Sometimes discussion of specific > issues/bugs/problems/... > This is something which happens frequently on the ARM call and seems to work > very well > > I would suggest to start with a 1 hour monthly meeting: possibly every 2nd > Tue or Thu each month (depends on timing). I know that people are spread > across different timezones (from China to the US), so I would like to gather > thoughts before choosing a time. We may have to have alternating time-slots > every other month: but this is not ideal for some. > > To do this, please > * Raise your hands on whether you or your org would want to participate \o/ > * Provide your timezone CT > * Provide a UTC time range when you can attend 15:00-23:00 > Your sincerely, > Lars > > -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/2] x86/svm: enable pause filtering threshold
If available, enable the pause filtering threshold feature. See the
previous commit for more information.
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/svm.c | 1 +
xen/arch/x86/hvm/svm/vmcb.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 9f58afc2d8..b6b92365bf 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1694,6 +1694,7 @@ const struct hvm_function_table * __init start_svm(void)
P(cpu_has_svm_vloadsave, "Virtual VMLOAD/VMSAVE");
P(cpu_has_svm_vgif, "Virtual GIF");
P(cpu_has_pause_filter, "Pause-Intercept Filter");
+P(cpu_has_pause_thresh, "Pause-Intercept Filter Threshold");
P(cpu_has_tsc_ratio, "TSC Rate MSR");
#undef P
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..ebe6f0c751 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -227,6 +227,9 @@ static int construct_vmcb(struct vcpu *v)
{
vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
+
+if ( cpu_has_pause_thresh )
+vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
}
vmcb->cleanbits.bytes = 0;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] x86/svm: add support for pause filtering threshold
Add support for enabling the pause filtering threshold feature. This
causes the pause filtering count to reset if there's pause filtering
threshold cycles or greater between pauses. See AMD APM Vol 2 Section
15.14.4 for more details.
The values of the pause filtering count and threshold were found by
iterating over different values of the count and threshold while running
kernbench and a pi spigot algorithm with yields placed in it. A
balanced setting for both variable provides:
(Using averaged elapsed time with kernbench)
old = 852.0
new = 848.8
improvement = .4%
For system without pause filtering threshold, the change, from 3000 to
4000 for the count, should not negatively effect system performance.
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/include/asm-x86/hvm/svm/svm.h | 5 -
xen/include/asm-x86/hvm/svm/vmcb.h | 3 ++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-x86/hvm/svm/svm.h
b/xen/include/asm-x86/hvm/svm/svm.h
index 462cb89b7c..593546fb56 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -64,6 +64,7 @@ extern u32 svm_feature_flags;
#define SVM_FEATURE_FLUSHBYASID6 /* TLB flush by ASID support */
#define SVM_FEATURE_DECODEASSISTS 7 /* Decode assists support */
#define SVM_FEATURE_PAUSEFILTER 10 /* Pause intercept filter support */
+#define SVM_FEATURE_PAUSETHRESH 12 /* Pause intercept filter support */
#define SVM_FEATURE_VLOADSAVE 15 /* virtual vmload/vmsave */
#define SVM_FEATURE_VGIF 16 /* Virtual GIF */
@@ -76,10 +77,12 @@ extern u32 svm_feature_flags;
#define cpu_has_svm_decodecpu_has_svm_feature(SVM_FEATURE_DECODEASSISTS)
#define cpu_has_svm_vgif cpu_has_svm_feature(SVM_FEATURE_VGIF)
#define cpu_has_pause_filter cpu_has_svm_feature(SVM_FEATURE_PAUSEFILTER)
+#define cpu_has_pause_thresh cpu_has_svm_feature(SVM_FEATURE_PAUSETHRESH)
#define cpu_has_tsc_ratio cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR)
#define cpu_has_svm_vloadsave cpu_has_svm_feature(SVM_FEATURE_VLOADSAVE)
-#define SVM_PAUSEFILTER_INIT3000
+#define SVM_PAUSEFILTER_INIT4000
+#define SVM_PAUSETHRESH_INIT1000
/* TSC rate */
#define DEFAULT_TSC_RATIO 0x0001ULL
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
b/xen/include/asm-x86/hvm/svm/vmcb.h
index 9d5dfc58f2..de07429dff 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -412,7 +412,7 @@ struct vmcb_struct {
u64 res04; /* offset 0x28 */
u64 res05; /* offset 0x30 */
u32 res06; /* offset 0x38 */
-u16 res06a; /* offset 0x3C */
+u16 _pause_filter_thresh; /* offset 0x3C - cleanbit 0 */
u16 _pause_filter_count;/* offset 0x3E - cleanbit 0 */
u64 _iopm_base_pa; /* offset 0x40 - cleanbit 1 */
u64 _msrpm_base_pa; /* offset 0x48 - cleanbit 1 */
@@ -568,6 +568,7 @@ VMCB_ACCESSORS(exception_intercepts, intercepts)
VMCB_ACCESSORS(general1_intercepts, intercepts)
VMCB_ACCESSORS(general2_intercepts, intercepts)
VMCB_ACCESSORS(pause_filter_count, intercepts)
+VMCB_ACCESSORS(pause_filter_thresh, intercepts)
VMCB_ACCESSORS(tsc_offset, intercepts)
VMCB_ACCESSORS(iopm_base_pa, iopm)
VMCB_ACCESSORS(msrpm_base_pa, iopm)
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/2] x86/svm: add pause filtering threshold for SVM
This patch series adds support and enablement of the pause filtering threshold. Once there's pause filtering threshold amount of cycles between pauses, the pause filtering counter resets to what was in the VMCB. This allows the pause filtering count to "reset" between pauses and keeps the guset from getting intercepted by the hypervisor. See AMD APM vol 2 section 15.14.4 for more details. In reply to this will be an email with graphs showing some benchmark results of why the values of the counter and threshold were picked. Brian Woods (2): x86/svm: add support for pause filtering threshold x86/svm: enable pause filtering threshold xen/arch/x86/hvm/svm/svm.c | 1 + xen/arch/x86/hvm/svm/vmcb.c| 3 +++ xen/include/asm-x86/hvm/svm/svm.h | 5 - xen/include/asm-x86/hvm/svm/vmcb.h | 3 ++- 4 files changed, 10 insertions(+), 2 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v4 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/nestedsvm.c| 66 +
xen/arch/x86/hvm/svm/svm.c | 6 +++
xen/arch/x86/hvm/svm/vmcb.c | 17 -
xen/include/asm-x86/hvm/svm/nestedsvm.h | 1 +
4 files changed, 73 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..9295e583d7 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1659,3 +1659,69 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs,
struct vcpu *v)
__update_guest_eip(regs, inst_len);
}
+
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+struct nestedsvm *svm = _nestedsvm(v);
+u32 general2_intercepts;
+vintr_t vintr;
+
+/*
+ * Need state for transfering the nested gif status so only write on
+ * the hvm_vcpu EFER.SVME changing.
+ */
+if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+{
+if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+ paging_mode_hap(v->domain) &&
+ cpu_has_svm_vloadsave )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 1;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+ GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( !vmcb->_vintr.fields.vgif_enable &&
+ cpu_has_svm_vgif )
+{
+vintr = vmcb_get_vintr(vmcb);
+vintr.fields.vgif = svm->ns_gif;
+vintr.fields.vgif_enable = 1;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+ GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+else
+{
+if ( vmcb->virt_ext.fields.vloadsave_enable )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 0;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( vmcb->_vintr.fields.vgif_enable )
+{
+vintr = vmcb_get_vintr(vmcb);
+svm->ns_gif = vintr.fields.vgif;
+vintr.fields.vgif_enable = 0;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..89140f02f3 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
if ( lma )
new_efer |= EFER_LME;
vmcb_set_efer(vmcb, new_efer);
+
+ASSERT(nestedhvm_enabled(v->domain) ||
+ !(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+if ( nestedhvm_enabled(v->domain) )
+svm_nested_features_on_efer_update(v);
}
static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
/* PAT is under complete control of SVM when using nested paging. */
svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
-{
-vmcb->virt_ext.fields.vloadsave_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-}
}
else
{
vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
}
-/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
-{
-vmcb->_vintr.fields.vgif = 1;
-vmcb->_vintr.fields.vgif_enable = 1;
-
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On Tue, Feb 20, 2018 at 05:09:24PM -0500, Boris Ostrovsky wrote: > That's possibly because you needed an SVM maintainer ACK. > > I think Jan was waiting for decision on how to present the ASSERT. From > the 3 options I slightly more prefer > > ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & > EFER_SVME)); > > (which wasn't the first choice for either of you ;-)) > > -boris I'll change it and send out a new version then. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
I've seen patch 1 and 3 are in but this one isn't. Any status on it? -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/nestedsvm.c| 66 +
xen/arch/x86/hvm/svm/svm.c | 6 +++
xen/arch/x86/hvm/svm/vmcb.c | 17 -
xen/include/asm-x86/hvm/svm/nestedsvm.h | 1 +
4 files changed, 73 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..9295e583d7 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1659,3 +1659,69 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs,
struct vcpu *v)
__update_guest_eip(regs, inst_len);
}
+
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+struct nestedsvm *svm = _nestedsvm(v);
+u32 general2_intercepts;
+vintr_t vintr;
+
+/*
+ * Need state for transfering the nested gif status so only write on
+ * the hvm_vcpu EFER.SVME changing.
+ */
+if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+{
+if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+ paging_mode_hap(v->domain) &&
+ cpu_has_svm_vloadsave )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 1;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+ GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( !vmcb->_vintr.fields.vgif_enable &&
+ cpu_has_svm_vgif )
+{
+vintr = vmcb_get_vintr(vmcb);
+vintr.fields.vgif = svm->ns_gif;
+vintr.fields.vgif_enable = 1;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+ GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+else
+{
+if ( vmcb->virt_ext.fields.vloadsave_enable )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 0;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( vmcb->_vintr.fields.vgif_enable )
+{
+vintr = vmcb_get_vintr(vmcb);
+svm->ns_gif = vintr.fields.vgif;
+vintr.fields.vgif_enable = 0;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..be08a5aa5e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -611,6 +611,12 @@ static void svm_update_guest_efer(struct vcpu *v)
if ( lma )
new_efer |= EFER_LME;
vmcb_set_efer(vmcb, new_efer);
+
+if ( !nestedhvm_enabled(v->domain) )
+ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+if ( nestedhvm_enabled(v->domain) )
+svm_nested_features_on_efer_update(v);
}
static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
/* PAT is under complete control of SVM when using nested paging. */
svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
-{
-vmcb->virt_ext.fields.vloadsave_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-}
}
else
{
vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
}
-/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
-{
-vmcb->_vintr.fields.vgif = 1;
-vmcb->_vintr.fields.vgif_enable = 1;
-
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On Thu, Feb 08, 2018 at 02:45:31AM -0700, Jan Beulich wrote: > I'm afraid I continue to be confused: A function with this name should > imo, as said earlier, live in nestedsvm.c. However ... I'll move it to nestedsvm.c then. > ... this indicates that the function does something even for the > non-nested case. In particular ... It makes sure that SVME isn't set when nested isn't enabled. If SVME is set it does certain checks to enable features if enabled. Else it makes sure nested features are turned off. The reason for this is that, with VGIF/VVMLOAD, they can still work even with SVME not being set. This sets it where they get intercepted to Xen so that Xen can correctly emulate what should be happening. If SVME isn't set then nested guest shouldn't be able to do VGIF/VVMLOAD. > ... this entire else block. Is it necessary to do this in the non-nested > case? IOW - do these settings ever change there (I would have > thought that the two *_enable fields checked by the two if()s should > never be true for nested-disabled guests)? Otherwise, as also said > before, the caller should call here only when > nestedhvm_enabled(v->domain), and the function would better > move. > > Jan It only checks two things (two if statements) in the VMCB per EFER update. Suppose you add an if to check if it's a nested guest and then run the nested_features func. You're only saving a total of one if and going in and out a function but you add a small divergence in the codepath. If this was a long computer/IO intense function, I'd completely agree but this is two very simple checks. I'll change it though, since it'll be easier than going back and forth about a minor detail. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
If Andy wants to use his version of this, that's fine also. This is just a newer version based on Jan's input. v1 -> v2 Got rid of "== X"s Added an assert and got rid of a check in an if -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/svm.c | 71 +
xen/arch/x86/hvm/svm/vmcb.c | 17 ---
2 files changed, 71 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..3e9c81b0e4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -601,6 +601,75 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
}
}
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+static void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+struct nestedsvm *svm = _nestedsvm(v);
+u32 general2_intercepts;
+vintr_t vintr;
+
+if ( !nestedhvm_enabled(v->domain) )
+ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));
+
+/*
+ * Need state for transfering the nested gif status so only write on
+ * the hvm_vcpu EFER.SVME changing.
+ */
+if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
+{
+if ( !vmcb->virt_ext.fields.vloadsave_enable &&
+ paging_mode_hap(v->domain) &&
+ cpu_has_svm_vloadsave )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 1;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+ GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( !vmcb->_vintr.fields.vgif_enable &&
+ cpu_has_svm_vgif )
+{
+vintr = vmcb_get_vintr(vmcb);
+vintr.fields.vgif = svm->ns_gif;
+vintr.fields.vgif_enable = 1;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+ GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+else
+{
+if ( vmcb->virt_ext.fields.vloadsave_enable )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 0;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( vmcb->_vintr.fields.vgif_enable )
+{
+vintr = vmcb_get_vintr(vmcb);
+svm->ns_gif = vintr.fields.vgif;
+vintr.fields.vgif_enable = 0;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+}
+
static void svm_update_guest_efer(struct vcpu *v)
{
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -611,6 +680,8 @@ static void svm_update_guest_efer(struct vcpu *v)
if ( lma )
new_efer |= EFER_LME;
vmcb_set_efer(vmcb, new_efer);
+
+svm_nested_features_on_efer_update(v);
}
static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
/* PAT is under complete control of SVM when using nested paging. */
svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
-{
-vmcb->virt_ext.fields.vloadsave_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-}
}
else
{
vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
}
-/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
-{
-vmcb->_vintr.fields.vgif = 1;
-vmcb->_vintr.fields.vgif_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-}
-
if ( cpu_has_pause_filter )
{
vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On Mon, Feb 05, 2018 at 03:37:06PM +, Andrew Cooper wrote: > Indenting is off, but that can be fixed on commit. Oopsy, sorry about that. > As some extra cleanup, what about folding this diff in? It avoids > repeatedly hitting the cleanbits, and is clearer to follow IMO. > > ~Andrew > It's functionally the same. It's a trade off of whether you want to write/read to the VMCB twice (assuming both are changing) or go through more if statements every time. The clean bits are just a field in the VMCB so. Some of the VMCB fields are only read on change with my version but to be honest, I don't think it matters either way. I like the locality of mine better, but I'm perfectly fine with either. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/3] x86/svm: update VGIF support
There are places where the GIF value is checked. A guest with VGIF
enabled can change the GIF value without the host being involved,
therefore it needs to check the GIF value in the VMCB rather the one in
the nestedsvm struct.
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/nestedsvm.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index b6f6449d75..1f7b0d3e88 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -800,8 +800,13 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct
cpu_user_regs *regs,
struct nestedvcpu *nv = _nestedhvm(v);
struct nestedsvm *svm = _nestedsvm(v);
struct vmcb_struct *ns_vmcb;
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+if ( vmcb->_vintr.fields.vgif_enable )
+ASSERT(vmcb->_vintr.fields.vgif == 0);
+else
+ASSERT(svm->ns_gif == 0);
-ASSERT(svm->ns_gif == 0);
ns_vmcb = nv->nv_vvmcx;
if (nv->nv_vmexit_pending) {
@@ -1343,8 +1348,13 @@ nestedsvm_vmexit_defer(struct vcpu *v,
uint64_t exitcode, uint64_t exitinfo1, uint64_t exitinfo2)
{
struct nestedsvm *svm = _nestedsvm(v);
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+if ( vmcb->_vintr.fields.vgif_enable )
+vmcb->_vintr.fields.vgif = 0;
+else
+nestedsvm_vcpu_clgi(v);
-nestedsvm_vcpu_clgi(v);
svm->ns_vmexit.exitcode = exitcode;
svm->ns_vmexit.exitinfo1 = exitinfo1;
svm->ns_vmexit.exitinfo2 = exitinfo2;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/3] x86/svm: correct EFER.SVME intercept checks
Corrects some EFER.SVME checks in intercepts. See AMD APM vol2 section
15.4 for more details. VMMCALL isn't checked due to guests needing it
to boot.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/nestedsvm.c | 10 --
xen/arch/x86/hvm/svm/svm.c | 8 +---
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 1f7b0d3e88..1f5981fc18 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1620,7 +1620,12 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs,
struct vcpu *v)
{
unsigned int inst_len;
-if ( !nestedhvm_enabled(v->domain) ) {
+/*
+ * STGI doesn't require SVME to be set to be used. See AMD APM vol
+ * 2 section 15.4 for details.
+ */
+if ( !nestedhvm_enabled(v->domain) )
+{
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
return;
}
@@ -1640,7 +1645,8 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs,
struct vcpu *v)
uint32_t general1_intercepts = vmcb_get_general1_intercepts(vmcb);
vintr_t intr;
-if ( !nestedhvm_enabled(v->domain) ) {
+if ( !nsvm_efer_svm_enabled(v) )
+{
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
return;
}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 7864ee39ae..2599f02ab6 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2255,7 +2255,6 @@ svm_vmexit_do_vmrun(struct cpu_user_regs *regs,
{
if ( !nsvm_efer_svm_enabled(v) )
{
-gdprintk(XENLOG_ERR, "VMRUN: nestedhvm disabled, injecting #UD\n");
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
return;
}
@@ -2310,7 +2309,6 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
if ( !nsvm_efer_svm_enabled(v) )
{
-gdprintk(XENLOG_ERR, "VMLOAD: nestedhvm disabled, injecting #UD\n");
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
return;
}
@@ -2346,7 +2344,6 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
if ( !nsvm_efer_svm_enabled(v) )
{
-gdprintk(XENLOG_ERR, "VMSAVE: nestedhvm disabled, injecting #UD\n");
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
return;
}
@@ -2820,6 +2817,11 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
break;
case VMEXIT_INVLPGA:
+if ( !nsvm_efer_svm_enabled(v) )
+{
+hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+break;
+}
if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 )
break;
svm_invlpga_intercept(v, regs->rax, regs->ecx);
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
Only enable virtual VMLOAD/SAVE and VGIF if the guest EFER.SVME is set.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/svm.c | 69 +
xen/arch/x86/hvm/svm/vmcb.c | 17 ---
2 files changed, 69 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfaa5d..7864ee39ae 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -601,6 +601,73 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
}
}
+/*
+ * This runs on EFER change to see if nested features need to either be
+ * turned off or on.
+ */
+static void svm_nested_features_on_efer_update(struct vcpu *v)
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+struct nestedsvm *svm = _nestedsvm(v);
+u32 general2_intercepts;
+vintr_t vintr;
+
+ /*
+* Need state for transfering the nested gif status so only write on
+* the hvm_vcpu EFER.SVME changing.
+*/
+if ( (v->arch.hvm_vcpu.guest_efer & EFER_SVME) &&
+ nestedhvm_enabled(v->domain))
+{
+if ( (vmcb->virt_ext.fields.vloadsave_enable == 0) &&
+ paging_mode_hap(v->domain) &&
+ cpu_has_svm_vloadsave )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 1;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
+ GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( (vmcb->_vintr.fields.vgif_enable == 0) &&
+ cpu_has_svm_vgif )
+{
+vintr = vmcb_get_vintr(vmcb);
+vintr.fields.vgif = svm->ns_gif;
+vintr.fields.vgif_enable = 1;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
+ GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+else
+{
+if ( vmcb->virt_ext.fields.vloadsave_enable == 1 )
+{
+vmcb->virt_ext.fields.vloadsave_enable = 0;
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
+GENERAL2_INTERCEPT_VMSAVE);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+
+if ( vmcb->_vintr.fields.vgif_enable == 1 )
+{
+vintr = vmcb_get_vintr(vmcb);
+svm->ns_gif = vintr.fields.vgif;
+vintr.fields.vgif_enable = 0;
+vmcb_set_vintr(vmcb, vintr);
+general2_intercepts = vmcb_get_general2_intercepts(vmcb);
+general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
+GENERAL2_INTERCEPT_CLGI);
+vmcb_set_general2_intercepts(vmcb, general2_intercepts);
+}
+}
+}
+
static void svm_update_guest_efer(struct vcpu *v)
{
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -611,6 +678,8 @@ static void svm_update_guest_efer(struct vcpu *v)
if ( lma )
new_efer |= EFER_LME;
vmcb_set_efer(vmcb, new_efer);
+
+svm_nested_features_on_efer_update(v);
}
static void svm_cpuid_policy_changed(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..997e7597e0 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -200,29 +200,12 @@ static int construct_vmcb(struct vcpu *v)
/* PAT is under complete control of SVM when using nested paging. */
svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
-
-/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
-{
-vmcb->virt_ext.fields.vloadsave_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMSAVE;
-}
}
else
{
vmcb->_exception_intercepts |= (1U << TRAP_page_fault);
}
-/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
-{
-vmcb->_vintr.fields.vgif = 1;
-vmcb->_vintr.fields.vgif_enable = 1;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_STGI;
-vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_CLGI;
-}
-
if ( cpu_has_pause_filter )
{
vmcb->_pause_filter_count = SVM_PAUSEFILTER_INIT;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/3] Various SVM Cleanups
This is a small series of SVM cleanup patches. The first patch is correcting a couple of checks relating to VGIF. The other two are ensuring that nested SVM functionality is emulated/setup more correctly. Brian Woods (3): x86/svm: update VGIF support x86/svm: add EFER SVME support for VGIF/VLOAD x86/svm: correct EFER.SVME intercept checks xen/arch/x86/hvm/svm/nestedsvm.c | 24 ++--- xen/arch/x86/hvm/svm/svm.c | 77 ++-- xen/arch/x86/hvm/svm/vmcb.c | 17 - 3 files changed, 94 insertions(+), 24 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/svm: Add checks for nested HW features
On Fri, Dec 22, 2017 at 03:15:48PM +, Andrew Cooper wrote: > Unfortunately, nestedhvm_enabled() is guaranteed to be false at the > point that construct_vmcb() is called (due the order in which > information appears while constructing the VM), which means we will > never enable these optimisations. > > Combined with the observation of EFER in the pipeline, the logic to > enable/disable these optimisations needs to be in > svm_update_guest_efer(), and need to trigger when EFER.SVME changes. > > ~Andrew Sorry for the late reply. I tired working this before vacation but it turned out to be a little bit longer than that... I have a set of patches that _should_ work, but there are other issues. Turns out there are interrupt issues with nestted SVM HVM and I'm trying to hunt those down and fix them so I can properly test the patches I've done. Oddly enough you can at least get a system booted on 17h family systems even if it's fragile but 15h just fails to even boot. Not sure how it even worked when I tested previous patches on the 15h system. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/svm: Add SVME checking for SVM intercepts
On Thu, Dec 21, 2017 at 11:52:27PM -0500, Boris Ostrovsky wrote: > s/it/is Oopsy, thank you. > I haven't checked all of them but at least for the first two > (svm_vmexit_do_vmrun() and smv_vmexit_do_vmload()) we check EFER and > print a similar error message. So it seems they can be handled in the > switch statement below. > > -boris They are indeed checked in those functions. I also need to check to make sure that the switching from L2 to L1 is done correctly in cause of SVME being low. I tested it with Xen and it worked correctly but it might not be failing correctly. The joys of being under the weather and forgetting all your mental notes. I'll try to send out a v2 of this patch today with the corrections. Thank you for your input. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] x86/svm: Add SVME checking for SVM intercepts
Checks the hvm EFER.SVME bit to make sure the EFER.SVME bit it high
before allowing nested SVM intercepts to be handled successfully. On
SVME being low, it generates a #UD as per the AMD APM vol2 15.4.
Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/svm.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2e62b9bb6d..2d0a82ae77 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2587,6 +2587,35 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
eventinj.fields.vector) )
vmcb->eventinj = eventinj;
+/*
+ * Making sure SVME is enabled see AMD APM vol2 section 15.4
+ * Nested Xen needs VMMCALL to boot. It hasn't set SVME by the time
+ * it uses it, therefore it isn't checked
+ */
+switch ( exit_reason )
+{
+case VMEXIT_VMRUN:
+case VMEXIT_VMLOAD:
+case VMEXIT_VMSAVE:
+case VMEXIT_CLGI:
+case VMEXIT_INVLPGA:
+if ( !(nestedhvm_enabled(v->domain) &&
+ nsvm_efer_svm_enabled(v)) )
+{
+gdprintk(XENLOG_ERR, "nestedhvm nested/SVME disabled, injecting
#UD\n");
+hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+ goto out;
+}
+case VMEXIT_STGI:
+case VMEXIT_SKINIT:
+if ( !nestedhvm_enabled(v->domain) )
+{
+gdprintk(XENLOG_ERR, "nestedhvm nested disabled, injecting #UD\n");
+hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
+ goto out;
+}
+}
+
switch ( exit_reason )
{
case VMEXIT_INTR:
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/2] x86/svm: Add checks for nested HW features
Add a nestedhvm_enable() check to the existing checks for setting the
virtual GIF and virtual VMLOAD/VMSAVE features. If it isn't a nestedhvm
guest, do not enable the features in the VMCB.
Signed-off-by: Brian Woods <brian.wo...@amd.com>
---
xen/arch/x86/hvm/svm/vmcb.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 0e6cba5b7b..62f7d7f7dd 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -202,7 +203,7 @@ static int construct_vmcb(struct vcpu *v)
svm_disable_intercept_for_msr(v, MSR_IA32_CR_PAT);
/* Use virtual VMLOAD/VMSAVE if available. */
-if ( cpu_has_svm_vloadsave )
+if ( cpu_has_svm_vloadsave && nestedhvm_enabled(v->domain) )
{
vmcb->virt_ext.fields.vloadsave_enable = 1;
vmcb->_general2_intercepts &= ~GENERAL2_INTERCEPT_VMLOAD;
@@ -215,7 +216,7 @@ static int construct_vmcb(struct vcpu *v)
}
/* if available, enable and configure virtual gif */
-if ( cpu_has_svm_vgif )
+if ( cpu_has_svm_vgif && nestedhvm_enabled(v->domain) )
{
vmcb->_vintr.fields.vgif = 1;
vmcb->_vintr.fields.vgif_enable = 1;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/2] Various SVM Patches
This is small patch series which does some basic maintenance work in the SVM section. 1. Add support for checking the SVME bit before doing nested SVM operations 2. Add support for checking if nestedhvm is enabled before enabling nested SVM features in the VMCB. Brian Woods (2): x86/svm: Add SVME checking for SVM intercepts x86/svm: Add checks for nested HW features xen/arch/x86/hvm/svm/svm.c | 28 xen/arch/x86/hvm/svm/vmcb.c | 5 +++-- 2 files changed, 31 insertions(+), 2 deletions(-) -- 2.11.0 Brian Woods (2): x86/svm: Add SVME checking for SVM intercepts x86/svm: Add checks for nested HW features xen/arch/x86/hvm/svm/svm.c | 29 + xen/arch/x86/hvm/svm/vmcb.c | 5 +++-- 2 files changed, 32 insertions(+), 2 deletions(-) -- 2.11.0 Brian Woods (2): x86/svm: Add SVME checking for SVM intercepts x86/svm: Add checks for nested HW features xen/arch/x86/hvm/svm/svm.c | 29 + xen/arch/x86/hvm/svm/vmcb.c | 5 +++-- 2 files changed, 32 insertions(+), 2 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
