[PATCH v3 10/10] driver/char: add RX support to the XHCI driver
Add another work ring buffer for received data, and point IN TRB at it.
Ensure there is always at least one pending IN TRB, so the controller
has a way to send incoming data to the driver.
Note that both "success" and "short packet" completion codes are okay -
in fact it will be "short packet" most of the time, as the TRB length is
about maximum size, not required size.
Signed-off-by: Marek Marczykowski-Górecki
---
New patch in v3
---
docs/misc/xen-command-line.pandoc | 4 +-
xen/drivers/char/xhci-dbc.c | 121 +++-
2 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index cc1e1989b17e..07174badac8f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -729,8 +729,8 @@ Available alternatives, with their meaning, are:
Specify the USB controller to use, either by instance number (when going
over the PCI busses sequentially) or by PCI device (must be on segment 0).
-Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability (output
-only). XHCI driver will wait indefinitely for the debug host to connect - make
+Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability.
+XHCI driver will wait indefinitely for the debug host to connect - make
sure the cable is connected.
The `share` option for xhci controls who else can use the controller:
* `none`: use the controller exclusively for console, even hardware domain
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 805b447f2300..ccf4f9bbe2b7 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -109,6 +109,7 @@ enum {
enum {
XHCI_TRB_CC_SUCCESS = 1,
XHCI_TRB_CC_TRB_ERR = 5,
+XHCI_TRB_CC_SHORT_PACKET = 13,
};
/* DbC endpoint types */
@@ -243,6 +244,7 @@ struct dbc {
struct xhci_trb_ring dbc_oring;
struct xhci_trb_ring dbc_iring;
struct dbc_work_ring dbc_owork;
+struct dbc_work_ring dbc_iwork;
struct xhci_string_descriptor *dbc_str;
pci_sbdf_t sbdf;
@@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
trb->ctrl |= 0x20;
}
+static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb)
+{
+return trb->params;
+}
+
+static uint32_t xhci_trb_norm_len(struct xhci_trb *trb)
+{
+return trb->status & 0x1;
+}
+
/**
* Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
* TRBs are read-only from software
@@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb
*trb)
return trb->status >> 24;
}
+/* Amount of data _not_ transferred */
+static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb)
+{
+return trb->status & 0x1;
+}
+
/* Fields for link TRBs (section 6.4.4.1) */
static void xhci_trb_link_set_rsp(struct xhci_trb *trb, uint64_t rsp)
{
@@ -495,6 +513,14 @@ static int xhci_trb_ring_full(const struct xhci_trb_ring
*ring)
return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq;
}
+static int xhci_trb_ring_size(const struct xhci_trb_ring *ring)
+{
+if ( ring->enq >= ring->deq )
+return ring->enq - ring->deq;
+
+return DBC_TRB_RING_CAP - ring->deq + ring->enq;
+}
+
static int dbc_work_ring_full(const struct dbc_work_ring *ring)
{
return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq;
@@ -508,6 +534,14 @@ static uint64_t dbc_work_ring_size(const struct
dbc_work_ring *ring)
return DBC_WORK_RING_CAP - ring->deq + ring->enq;
}
+static uint64_t dbc_work_ring_space_to_end(const struct dbc_work_ring *ring)
+{
+if ( ring->enq >= ring->deq )
+return DBC_WORK_RING_CAP - ring->enq;
+
+return ring->deq - ring->enq;
+}
+
static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring,
uint64_t dma, uint64_t len)
{
@@ -568,6 +602,31 @@ static int64_t dbc_push_work(struct dbc *dbc, struct
dbc_work_ring *ring,
return i;
}
+static void dbc_rx_trb(struct dbc *dbc, struct xhci_trb *trb,
+ uint64_t not_transferred)
+{
+struct dbc_work_ring *ring = >dbc_iwork;
+unsigned int rx_len;
+unsigned int end, start = ring->enq;
+
+if ( xhci_trb_type(trb) != XHCI_TRB_NORM )
+/* Can be Link TRB for example. */
+return;
+
+ASSERT(xhci_trb_norm_buf(trb) == ring->dma + ring->enq);
+ASSERT(xhci_trb_norm_len(trb) >= not_transferred);
+rx_len = xhci_trb_norm_len(trb) - not_transferred;
+
+/* It can hit the ring end, but should not wrap around. */
+ASSERT(ring->enq + rx_len <= DBC_WORK_RING_CAP);
+ring->enq = (ring->enq + rx_len) & (DBC_WORK_RING_CAP - 1);
+
+end = ring->enq;
+
+if ( end > start )
+cache_flush(>buf[start], end - start);
+}
+
/*
* Note that if IN transfer support is added, then this
* will need to be changed; it assumes an OUT transfer ring only
@@ -577,6 +636,7 @@ static void dbc_pop_events(struct dbc
[PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
That's possible, because the capability was designed specifically to
allow separate driver handle it, in parallel to unmodified xhci driver
(separate set of registers, pretending the port is "disconnected" for
the main xhci driver etc). It works with Linux dom0, although requires
an awful hack - re-enabling bus mastering behind dom0's backs.
Linux driver does similar thing - see
drivers/usb/early/xhci-dbc.c:xdbc_handle_events().
Add an option to allow/deny other domains to use the USB controller. By
default, if XHCI console is enabled, Xen will take the whole controller
for itself, using `dbgp=xhci,share=hwdom` or `=any` allows other ports
to be used by either only dom0 or any dom0 that get this PCI device
assigned.
In any case, to avoid Linux messing with the DbC, mark this MMIO area as
read-only.
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- adjust for xhci-dbc rename
- adjust for dbc_ensure_running() split
- wrap long lines
- add runtime option for sharing USB controller
---
docs/misc/xen-command-line.pandoc | 12 ++-
xen/drivers/char/xhci-dbc.c | 115 ---
2 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index e53efdb324b3..cc1e1989b17e 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -724,7 +724,7 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ | @pci:. ]`
-> `= xhci[ | @pci:. ]`
+> `= xhci[ | @pci:. ][,share=none|hwdom|any]`
Specify the USB controller to use, either by instance number (when going
over the PCI busses sequentially) or by PCI device (must be on segment 0).
@@ -732,6 +732,16 @@ over the PCI busses sequentially) or by PCI device (must
be on segment 0).
Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability (output
only). XHCI driver will wait indefinitely for the debug host to connect - make
sure the cable is connected.
+The `share` option for xhci controls who else can use the controller:
+* `none`: use the controller exclusively for console, even hardware domain
+ (dom0) cannot use it; this is the default
+* `hwdom`: hardware domain may use the controller too, ports not used for debug
+ console will be available for normal devices
+* `any`: the controller can be assigned to any domain; it is not safe to assign
+ the controller to untrusted domain
+
+Choosing `share=hwdom` or `share=any` allows a domain to reset the controller,
+which may cause small portion of the console output to be lost.
### debug_stack_lines
> `= `
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 546231a75894..805b447f2300 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -228,6 +229,12 @@ struct dbc_work_ring {
uint64_t dma;
};
+enum xhci_share {
+XHCI_SHARE_NONE = 0,
+XHCI_SHARE_HWDOM,
+XHCI_SHARE_ANY
+};
+
struct dbc {
struct dbc_reg __iomem *dbc_reg;
struct xhci_dbc_ctx *dbc_ctx;
@@ -244,6 +251,7 @@ struct dbc {
void __iomem *xhc_mmio;
bool open;
+enum xhci_share share;
unsigned int xhc_num; /* look for n-th xhc */
};
@@ -871,8 +879,9 @@ static bool __init dbc_open(struct dbc *dbc)
}
/*
- * Ensure DbC is still running, handle events, and possibly re-enable if cable
- * was re-plugged. Returns true if DbC is operational.
+ * Ensure DbC is still running, handle events, and possibly
+ * re-enable/re-configure if cable was re-plugged or controller was reset.
+ * Returns true if DbC is operational.
*/
static bool dbc_ensure_running(struct dbc *dbc)
{
@@ -880,6 +889,42 @@ static bool dbc_ensure_running(struct dbc *dbc)
uint32_t ctrl;
uint32_t cmd;
+if ( dbc->share != XHCI_SHARE_NONE )
+{
+/*
+ * Re-enable memory decoding and later bus mastering, if dom0 (or
+ * other) disabled it in the meantime.
+ */
+cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+if ( !(cmd & PCI_COMMAND_MEMORY) )
+{
+cmd |= PCI_COMMAND_MEMORY;
+pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+}
+
+if ( dbc->open && !(readl(>ctrl) & (1U << DBC_CTRL_DCE)) )
+{
+if ( !dbc_init_dbc(dbc) )
+return false;
+
+dbc_init_work_ring(dbc, >dbc_owork);
+dbc_enable_dbc(dbc);
+}
+else
+{
+/*
+ * dbc_init_dbc() takes care about it, so check only if it wasn't
+ * called.
+ */
+cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+if ( !(cmd & PCI_COMMAND_MASTER) )
+{
+cmd |= PCI_COMMAND_MASTER;
+pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+}
+}
+}
+
dbc_pop_events(dbc);
[PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI
The important part is to include those buffers in IOMMU page table
relevant for the USB controller. Otherwise, DbC will stop working as
soon as IOMMU is enabled, regardless of to which domain device assigned
(be it xen or dom0).
If the device is passed through to dom0 or other domain (see later
patches), that domain will effectively have access to those buffers too.
It does give such domain yet another way to DoS the system (as is the
case when having PCI device assigned already), but also possibly steal
the console ring content. Thus, such domain should be a trusted one.
In any case, prevent anything else being placed on those pages by adding
artificial padding.
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- adjust for xhci-dbc rename
- do not raise MAX_USER_RMRR_PAGES
- adjust alignment of DMA buffers
---
xen/drivers/char/xhci-dbc.c | 42 +-
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 14a2d3eb0ee2..546231a75894 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -20,6 +20,7 @@
*/
#include
+#include
#include
#include
#include
@@ -1046,13 +1047,20 @@ static struct uart_driver dbc_uart_driver = {
.flush = dbc_uart_flush,
};
-static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
-static struct xhci_erst_segment erst __aligned(64);
-static struct xhci_dbc_ctx ctx __aligned(64);
-static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
-static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+struct dbc_dma_bufs {
+struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
+struct xhci_trb out_trb[DBC_TRB_RING_CAP];
+struct xhci_trb in_trb[DBC_TRB_RING_CAP];
+uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
+struct xhci_erst_segment erst __aligned(16);
+struct xhci_dbc_ctx ctx __aligned(16);
+struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+/*
+ * Don't place anything else on this page - it will be
+ * DMA-reachable by the USB controller.
+ */
+};
+static struct dbc_dma_bufs dbc_dma_bufs __section(".bss.page_aligned");
static char __initdata opt_dbgp[30];
string_param("dbgp", opt_dbgp);
@@ -1087,16 +1095,22 @@ void __init xhci_dbc_uart_init(void)
dbc->sbdf = PCI_SBDF(0, bus, slot, func);
}
-dbc->dbc_ctx =
-dbc->dbc_erst =
-dbc->dbc_ering.trb = evt_trb;
-dbc->dbc_oring.trb = out_trb;
-dbc->dbc_iring.trb = in_trb;
-dbc->dbc_owork.buf = out_wrk_buf;
-dbc->dbc_str = str_buf;
+dbc->dbc_ctx = _dma_bufs.ctx;
+dbc->dbc_erst = _dma_bufs.erst;
+dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
+dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
+dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
+dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
+dbc->dbc_str = dbc_dma_bufs.str_buf;
if ( dbc_open(dbc) )
+{
+iommu_add_extra_reserved_device_memory(
+PFN_DOWN(virt_to_maddr(_dma_bufs)),
+PFN_UP(sizeof(dbc_dma_bufs)),
+uart->dbc.sbdf.sbdf);
serial_register_uart(SERHND_DBGP, _uart_driver, _uart);
+}
}
#ifdef DBC_DEBUG
--
git-series 0.9.1
[PATCH v3 07/10] IOMMU/AMD: wire common device reserved memory API
Register common device reserved memory similar to how ivmd= parameter is
handled.
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- use variable initializer
- use pfn_to_paddr()
---
xen/drivers/passthrough/amd/iommu_acpi.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
b/xen/drivers/passthrough/amd/iommu_acpi.c
index ac6835225bae..3b577c9b390c 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -1078,6 +1078,25 @@ static inline bool_t is_ivmd_block(u8 type)
type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
}
+static int __init cf_check add_one_extra_ivmd(unsigned long start,
+ unsigned long nr,
+ uint32_t id, void *ctxt)
+{
+struct acpi_ivrs_memory ivmd = {
+.header = {
+.length = sizeof(ivmd),
+.flags = ACPI_IVMD_UNITY | ACPI_IVMD_READ | ACPI_IVMD_WRITE,
+.device_id = id,
+.type = ACPI_IVRS_TYPE_MEMORY_ONE,
+},
+};
+
+ivmd.start_address = pfn_to_paddr(start);
+ivmd.memory_length = pfn_to_paddr(nr);
+
+return parse_ivmd_block();
+}
+
static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
{
const struct acpi_ivrs_header *ivrs_block;
@@ -1121,6 +1140,8 @@ static int __init cf_check parse_ivrs_table(struct
acpi_table_header *table)
AMD_IOMMU_DEBUG("IVMD: %u command line provided entries\n", nr_ivmd);
for ( i = 0; !error && i < nr_ivmd; ++i )
error = parse_ivmd_block(user_ivmds + i);
+if ( !error )
+error = iommu_get_extra_reserved_device_memory(add_one_extra_ivmd,
NULL);
/* Each IO-APIC must have been mentioned in the table. */
for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
--
git-series 0.9.1
[PATCH v3 06/10] IOMMU/VT-d: wire common device reserved memory API
Re-use rmrr= parameter handling code to handle common device reserved
memory.
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR
---
xen/drivers/passthrough/vtd/dmar.c | 201 +-
1 file changed, 119 insertions(+), 82 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/dmar.c
b/xen/drivers/passthrough/vtd/dmar.c
index 367304c8739c..3df5f6b69719 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -861,111 +861,139 @@ static struct user_rmrr __initdata
user_rmrrs[MAX_USER_RMRR];
/* Macro for RMRR inclusive range formatting. */
#define ERMRRU_FMT "[%lx-%lx]"
-#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn
+#define ERMRRU_ARG base_pfn, end_pfn
+
+static int __init add_one_user_rmrr(unsigned long base_pfn,
+unsigned long end_pfn,
+unsigned int dev_count,
+uint32_t *sbdf);
static int __init add_user_rmrr(void)
{
+unsigned int i;
+int ret;
+
+for ( i = 0; i < nr_rmrr; i++ )
+{
+ret = add_one_user_rmrr(user_rmrrs[i].base_pfn,
+user_rmrrs[i].end_pfn,
+user_rmrrs[i].dev_count,
+user_rmrrs[i].sbdf);
+if ( ret < 0 )
+return ret;
+}
+return 0;
+}
+
+/* Returns 1 on success, 0 when ignoring and < 0 on error. */
+static int __init add_one_user_rmrr(unsigned long base_pfn,
+unsigned long end_pfn,
+unsigned int dev_count,
+uint32_t *sbdf)
+{
struct acpi_rmrr_unit *rmrr, *rmrru;
-unsigned int idx, seg, i;
-unsigned long base, end;
+unsigned int idx, seg;
+unsigned long base_iter;
bool overlap;
-for ( i = 0; i < nr_rmrr; i++ )
+if ( iommu_verbose )
+printk(XENLOG_DEBUG VTDPREFIX
+ "Adding RMRR for %d device ([0]: %#x) range "ERMRRU_FMT"\n",
+ dev_count, sbdf[0], ERMRRU_ARG);
+
+if ( base_pfn > end_pfn )
{
-base = user_rmrrs[i].base_pfn;
-end = user_rmrrs[i].end_pfn;
+printk(XENLOG_ERR VTDPREFIX
+ "Invalid RMRR Range "ERMRRU_FMT"\n",
+ ERMRRU_ARG);
+return 0;
+}
-if ( base > end )
+overlap = false;
+list_for_each_entry(rmrru, _rmrr_units, list)
+{
+if ( pfn_to_paddr(base_pfn) <= rmrru->end_address &&
+ rmrru->base_address <= pfn_to_paddr(end_pfn) )
{
printk(XENLOG_ERR VTDPREFIX
- "Invalid RMRR Range "ERMRRU_FMT"\n",
- ERMRRU_ARG(user_rmrrs[i]));
-continue;
+ "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
+ ERMRRU_ARG,
+ paddr_to_pfn(rmrru->base_address),
+ paddr_to_pfn(rmrru->end_address));
+overlap = true;
+break;
}
+}
+/* Don't add overlapping RMRR. */
+if ( overlap )
+return 0;
-if ( (end - base) >= MAX_USER_RMRR_PAGES )
+base_iter = base_pfn;
+do
+{
+if ( !mfn_valid(_mfn(base_iter)) )
{
printk(XENLOG_ERR VTDPREFIX
- "RMRR range "ERMRRU_FMT" exceeds "\
- __stringify(MAX_USER_RMRR_PAGES)" pages\n",
- ERMRRU_ARG(user_rmrrs[i]));
-continue;
+ "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
+ ERMRRU_ARG);
+break;
}
+} while ( base_iter++ < end_pfn );
-overlap = false;
-list_for_each_entry(rmrru, _rmrr_units, list)
-{
-if ( pfn_to_paddr(base) <= rmrru->end_address &&
- rmrru->base_address <= pfn_to_paddr(end) )
-{
-printk(XENLOG_ERR VTDPREFIX
- "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
- ERMRRU_ARG(user_rmrrs[i]),
- paddr_to_pfn(rmrru->base_address),
- paddr_to_pfn(rmrru->end_address));
-overlap = true;
-break;
-}
-}
-/* Don't add overlapping RMRR. */
-if ( overlap )
-continue;
+/* Invalid pfn in range as the loop ended before end_pfn was reached. */
+if ( base_iter <= end_pfn )
+return 0;
-do
-{
-if ( !mfn_valid(_mfn(base)) )
-{
-printk(XENLOG_ERR VTDPREFIX
- "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
- ERMRRU_ARG(user_rmrrs[i]));
-break;
-}
-} while ( base++ < end );
+rmrr = xzalloc(struct acpi_rmrr_unit);
+if ( !rmrr )
+return -ENOMEM;
[PATCH v3 05/10] IOMMU: add common API for device reserved memory
Add API similar to rmrr= and ivmd= arguments, but in a common code. This
will allow drivers to register reserved memory regardless of the IOMMU
vendor.
The direct reason for this API is xhci-dbc console driver (aka xue),
that needs to use DMA. But future change may unify command line
arguments for user-supplied reserved memory, and it may be useful for
other drivers in the future too.
This commit just introduces an API, subsequent patches will plug it in
appropriate places. The reserved memory ranges needs to be saved
locally, because at the point when they are collected, Xen doesn't know
yet which IOMMU driver will be used.
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- adjust code style
---
xen/drivers/passthrough/iommu.c | 45 ++-
xen/include/xen/iommu.h | 13 ++-
2 files changed, 58 insertions(+)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 77f64e61748d..74efd865ab69 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -651,6 +651,51 @@ bool_t iommu_has_feature(struct domain *d, enum
iommu_feature feature)
return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
}
+#define MAX_EXTRA_RESERVED_RANGES 20
+struct extra_reserved_range {
+unsigned long start;
+unsigned long nr;
+uint32_t sbdf;
+};
+static unsigned int __initdata nr_extra_reserved_ranges;
+static struct extra_reserved_range __initdata
+extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
+
+int iommu_add_extra_reserved_device_memory(unsigned long start,
+ unsigned long nr,
+ uint32_t sbdf)
+{
+unsigned int idx;
+
+if ( nr_extra_reserved_ranges >= MAX_EXTRA_RESERVED_RANGES )
+return -ENOMEM;
+
+idx = nr_extra_reserved_ranges++;
+extra_reserved_ranges[idx].start = start;
+extra_reserved_ranges[idx].nr = nr;
+extra_reserved_ranges[idx].sbdf = sbdf;
+
+return 0;
+}
+
+int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+unsigned int idx;
+int ret;
+
+for ( idx = 0; idx < nr_extra_reserved_ranges; idx++ )
+{
+ret = func(extra_reserved_ranges[idx].start,
+ extra_reserved_ranges[idx].nr,
+ extra_reserved_ranges[idx].sbdf,
+ ctxt);
+if ( ret < 0 )
+return ret;
+}
+
+return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 79529adf1fa5..aa87c3fd9ebc 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -297,6 +297,19 @@ struct iommu_ops {
#endif
};
+/*
+ * To be called by Xen internally, to register extra RMRR/IVMD ranges.
+ * Needs to be called before IOMMU initialization.
+ */
+extern int iommu_add_extra_reserved_device_memory(unsigned long start,
+ unsigned long nr,
+ uint32_t sbdf);
+/*
+ * To be called by specific IOMMU driver during initialization,
+ * to fetch ranges registered with iommu_add_extra_reserved_device_memory().
+ */
+extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void
*ctxt);
+
#include
#ifndef iommu_call
--
git-series 0.9.1
[PATCH v3 03/10] drivers/char: add support for selecting specific xhci
Handle parameters similar to dbgp=ehci.
Implement this by not resettting dbc->sbdf again in dbc_init_xhc(), but
using a value found there if non-zero. Additionally, add xue->xhc_num to
select n-th controller.
Signed-off-by: Marek Marczykowski-Górecki
Reviewed-by: Jan Beulich
---
Changes in v3:
- adjust for xhci-dbc rename
- drop redundant check in parsing dbgp= option
Changes in v2:
- unsigned int xhc_num
- code style
---
docs/misc/xen-command-line.pandoc | 2 +-
xen/drivers/char/xhci-dbc.c | 54
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index f936283cd187..d594bd5c7436 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -721,7 +721,7 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ | @pci:. ]`
-> `= xhci`
+> `= xhci[ | @pci:. ]`
Specify the USB controller to use, either by instance number (when going
over the PCI busses sequentially) or by PCI device (must be on segment 0).
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 53c39eedd4a6..11026d3b71f0 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -243,6 +243,7 @@ struct dbc {
void __iomem *xhc_mmio;
bool open;
+unsigned int xhc_num; /* look for n-th xhc */
};
static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
@@ -274,24 +275,37 @@ static bool __init dbc_init_xhc(struct dbc *dbc)
uint16_t cmd;
size_t xhc_mmio_size;
-/*
- * Search PCI bus 0 for the xHC. All the host controllers supported so far
- * are part of the chipset and are on bus 0.
- */
-for ( devfn = 0; devfn < 256; devfn++ )
+if ( dbc->sbdf.sbdf == 0 )
{
-pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
-uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
-
-if ( hdr == 0 || hdr == 0x80 )
+/*
+ * Search PCI bus 0 for the xHC. All the host controllers supported so
+ * far are part of the chipset and are on bus 0.
+ */
+for ( devfn = 0; devfn < 256; devfn++ )
{
-if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) ==
DBC_XHC_CLASSC )
+pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
+uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+if ( hdr == 0 || hdr == 0x80 )
{
-dbc->sbdf = sbdf;
-break;
+if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) ==
+ DBC_XHC_CLASSC )
+{
+if ( dbc->xhc_num-- )
+continue;
+dbc->sbdf = sbdf;
+break;
+}
}
}
}
+else
+{
+/* Verify if selected device is really xHC */
+if ( (pci_conf_read32(dbc->sbdf, PCI_CLASS_REVISION) >> 8) !=
+ DBC_XHC_CLASSC )
+dbc->sbdf.sbdf = 0;
+}
if ( !dbc->sbdf.sbdf )
{
@@ -1047,12 +1061,28 @@ void __init xhci_dbc_uart_init(void)
{
struct dbc_uart *uart = _uart;
struct dbc *dbc = >dbc;
+const char *e;
if ( strncmp(opt_dbgp, "xhci", 4) )
return;
memset(dbc, 0, sizeof(*dbc));
+if ( isdigit(opt_dbgp[4]) )
+{
+dbc->xhc_num = simple_strtoul(opt_dbgp + 4, , 10);
+}
+else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
+{
+unsigned int bus, slot, func;
+
+e = parse_pci(opt_dbgp + 8, NULL, , , );
+if ( !e || *e )
+return;
+
+dbc->sbdf = PCI_SBDF(0, bus, slot, func);
+}
+
dbc->dbc_ctx =
dbc->dbc_erst =
dbc->dbc_ering.trb = evt_trb;
--
git-series 0.9.1
[PATCH v3 04/10] console: support multiple serial console simultaneously
Previously only one serial console was supported at the same time. Using
console=com1,dbgp,vga silently ignored all but last serial console (in
this case: only dbgp and vga were active).
Fix this by storing not a single sercon_handle, but an array of them, up
to MAX_SERCONS entries. The value of MAX_SERCONS can be chosen in
kconfig, the default (4) is arbitrary, inspired by the number of
SERHND_IDX values.
Make console_steal() aware of multiple consoles too. It can now either
steal output from specific console (for gdbstub), or from all of them at
once (for console suspend).
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- adjust console_steal() for multiple consoles too
- add MAX_SERCONS to kconfig
- add warning about sync_console impact
- add warning if too many consoles are configured
- log issue with PCI spec parsing
---
docs/misc/xen-command-line.pandoc | 4 +-
xen/drivers/char/Kconfig | 11 -
xen/drivers/char/console.c| 97
xen/drivers/char/xhci-dbc.c | 6 +-
xen/include/xen/serial.h | 1 +-
5 files changed, 95 insertions(+), 24 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index d594bd5c7436..e53efdb324b3 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -433,6 +433,9 @@ only available when used together with `pv-in-pvh`.
`none` indicates that Xen should not use a console. This option only
makes sense on its own.
+Specifying more than one serial console will increase console latency,
+especially when `sync_console` option is used.
+
### console_timestamps
> `= none | date | datems | boot | raw`
@@ -2372,6 +2375,7 @@ vulnerabilities.
Flag to force synchronous console output. Useful for debugging, but
not suitable for production environments due to incurred overhead.
+If multiple consoles are configured, the incurred overhead is even bigger.
### tboot (x86)
> `= 0x`
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index 06350c387371..1010436d245c 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -85,6 +85,17 @@ config SERIAL_TX_BUFSIZE
Default value is 16384 (16kiB).
+config MAX_SERCONS
+ int "Maximum number of serial consoles active at once"
+ default 4
+ help
+ Controls how many serial consoles can be active at once. Configuring more
+ using `console=` parameter will be ignored.
+ When multiple consoles are configured, overhead of `sync_console` option
+ is even bigger.
+
+ Default value is 4.
+
config XHCI
bool "XHCI DbC UART driver"
depends on X86
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f9937c5134c0..2ffc919445c6 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -113,7 +113,9 @@ static char *__read_mostly conring = _conring;
static uint32_t __read_mostly conring_size = _CONRING_SIZE;
static uint32_t conringc, conringp;
-static int __read_mostly sercon_handle = -1;
+#define MAX_SERCONS CONFIG_MAX_SERCONS
+static int __read_mostly sercon_handle[MAX_SERCONS];
+static int __read_mostly nr_sercon_handle = 0;
#ifdef CONFIG_X86
/* Tristate: 0 disabled, 1 user enabled, -1 default enabled */
@@ -393,32 +395,59 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
static char serial_rx_ring[SERIAL_RX_SIZE];
static unsigned int serial_rx_cons, serial_rx_prod;
-static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
+/* The last entry means "steal from all consoles" */
+static void (*serial_steal_fn[MAX_SERCONS+1])(const char *, size_t nr) = {
+[MAX_SERCONS] = early_puts,
+};
+/*
+ * Redirect console *handle* output to *fn*. Use SERHND_STEAL_ALL as *handle*
to
+ * redirect all the consoles.
+ */
int console_steal(int handle, void (*fn)(const char *, size_t nr))
{
-if ( (handle == -1) || (handle != sercon_handle) )
-return 0;
+int i;
+
+if ( handle == -1 )
+return -ENOENT;
+if ( serial_steal_fn[MAX_SERCONS] != NULL )
+return -EBUSY;
+if ( handle == SERHND_STEAL_ALL )
+{
+serial_steal_fn[MAX_SERCONS] = fn;
+return MAX_SERCONS;
+}
+for ( i = 0; i < nr_sercon_handle; i++ )
+if ( handle == sercon_handle[i] )
+break;
+if ( i == nr_sercon_handle )
+return -ENOENT;
-if ( serial_steal_fn != NULL )
+if ( serial_steal_fn[i] != NULL )
return -EBUSY;
-serial_steal_fn = fn;
-return 1;
+serial_steal_fn[i] = fn;
+return i;
}
void console_giveback(int id)
{
-if ( id == 1 )
-serial_steal_fn = NULL;
+if ( id >= 0 && id <= MAX_SERCONS )
+serial_steal_fn[id] = NULL;
}
void console_serial_puts(const char *s, size_t nr)
{
-if ( serial_steal_fn != NULL )
-serial_steal_fn(s, nr);
+int i;
+
+if ( serial_steal_fn[MAX_SERCONS] !=
[PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc
Reset ports, to force host system to re-enumerate devices. Otheriwse it
will require the cable to be re-plugged, or will wait in the
"configuring" state indefinitely.
Trick and code copied from Linux:
drivers/usb/early/xhci-dbc.c:xdbc_start()->xdbc_reset_debug_port()
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- adjust for renamed driver
- use readl() etc for MMIO
- simplify xcap lookup
- drop acked-by
Changes in v2:
- use uint32_t instead of u32
- code style
---
xen/drivers/char/xhci-dbc.c | 75 ++-
1 file changed, 75 insertions(+)
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index f0e60d1b86aa..53c39eedd4a6 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -63,6 +63,10 @@
((1UL << DBC_PSC_CSC) | (1UL << DBC_PSC_PRC) | (1UL << DBC_PSC_PLC) |
\
(1UL << DBC_PSC_CEC))
+#define XHC_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff)
+#define PORT_RESET (1 << 4)
+#define PORT_CONNECT (1 << 0)
+
#define dbc_debug(...) printk("dbc debug: " __VA_ARGS__)
#define dbc_alert(...) printk("dbc alert: " __VA_ARGS__)
#define dbc_error(...) printk("dbc error: " __VA_ARGS__)
@@ -660,6 +664,73 @@ static void dbc_init_strings(struct dbc *dbc, uint32_t
*info)
>dbc_ctx->serial_size);
}
+static void dbc_do_reset_debug_port(struct dbc *dbc,
+unsigned int id, unsigned int count)
+{
+uint32_t __iomem *ops_reg;
+uint32_t __iomem *portsc;
+uint32_t val, cap_length;
+unsigned int i;
+
+cap_length = readl(dbc->xhc_mmio) & 0xff;
+ops_reg = dbc->xhc_mmio + cap_length;
+
+id--;
+for ( i = id; i < (id + count); i++ )
+{
+portsc = ops_reg + 0x100 + i * 0x4;
+val = readl(portsc);
+if ( !(val & PORT_CONNECT) )
+writel(val | PORT_RESET, portsc);
+}
+}
+
+static void dbc_reset_debug_port(struct dbc *dbc)
+{
+uint32_t val, port_offset, port_count;
+uint32_t __iomem *xcap;
+uint32_t xcap_val;
+uint32_t next;
+uint32_t id;
+uint8_t __iomem *mmio = (uint8_t *)dbc->xhc_mmio;
+uint32_t __iomem *hccp1 = (uint32_t *)(mmio + 0x10);
+const uint32_t PROTOCOL_ID = 0x2;
+int ttl = 48;
+
+xcap = (uint32_t *)dbc->xhc_mmio;
+/*
+ * This is initially an offset to the first capability. All the offsets
+ * (both in HCCP1 and then next capability pointer are dword-based.
+ */
+next = (readl(hccp1) & 0x) >> 16;
+
+/*
+ * Look for "supported protocol" capability, major revision 3.
+ * There may be multiple of them.
+ */
+while ( next && ttl-- )
+{
+xcap += next;
+xcap_val = readl(xcap);
+id = xcap_val & 0xFF;
+next = (xcap_val & 0xFF00) >> 8;
+
+if ( id != PROTOCOL_ID )
+continue;
+
+if ( XHC_EXT_PORT_MAJOR(xcap_val) != 0x3 )
+continue;
+
+/* extract ports offset and count from the capability structure */
+val = readl(xcap + 2);
+port_offset = val & 0xff;
+port_count = (val >> 8) & 0xff;
+
+/* and reset them all */
+dbc_do_reset_debug_port(dbc, port_offset, port_count);
+}
+}
+
static void dbc_enable_dbc(struct dbc *dbc)
{
struct dbc_reg *reg = dbc->dbc_reg;
@@ -671,6 +742,10 @@ static void dbc_enable_dbc(struct dbc *dbc)
while ( (readl(>ctrl) & (1U << DBC_CTRL_DCE)) == 0 )
cpu_relax();
+/* reset ports on initial open, to force re-enumerating by the host */
+if ( !dbc->open )
+dbc_reset_debug_port(dbc);
+
wmb();
writel(readl(>portsc) | (1U << DBC_PSC_PED), >portsc);
wmb();
--
git-series 0.9.1
[PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability
This is integration of https://github.com/connojd/xue into mainline Xen. This patch series includes several patches that I made in the process, some are very loosely related. The driver developed by Connor supports console via USB3 debug capability. The capability is designed to operate mostly independently of normal XHCI driver, so this patch series allows dom0 to drive standard USB3 controller part, while Xen uses DbC for console output. Changes since RFC: - move the driver to xue.c, remove non-Xen parts, remove now unneeded abstraction - adjust for Xen code style - build for x86 only - drop patch hidding the device from dom0 Changes since v1: - drop ehci patch - already applied - adjust for review comments from Jan (see changelogs in individual patches) Changes since v2: - add runtime option to share (or not) the controller with dom0 or other domains - add RX support - several smaller changes according to review comments Cc: Andrew Cooper Cc: George Dunlap Cc: Jan Beulich Cc: Julien Grall Cc: Stefano Stabellini Cc: Wei Liu Cc: "Roger Pau Monné" Cc: Paul Durrant Cc: Kevin Tian Cc: Connor Davis Connor Davis (1): drivers/char: Add support for USB3 DbC debugger Marek Marczykowski-Górecki (9): drivers/char: reset XHCI ports when initializing dbc drivers/char: add support for selecting specific xhci console: support multiple serial console simultaneously IOMMU: add common API for device reserved memory IOMMU/VT-d: wire common device reserved memory API IOMMU/AMD: wire common device reserved memory API drivers/char: mark DMA buffers as reserved for the XHCI drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC driver/char: add RX support to the XHCI driver docs/misc/xen-command-line.pandoc| 19 +- xen/arch/x86/include/asm/fixmap.h|4 +- xen/arch/x86/setup.c |1 +- xen/drivers/char/Kconfig | 20 +- xen/drivers/char/Makefile|1 +- xen/drivers/char/console.c | 97 +- xen/drivers/char/xhci-dbc.c | 1367 +++- xen/drivers/passthrough/amd/iommu_acpi.c | 21 +- xen/drivers/passthrough/iommu.c | 45 +- xen/drivers/passthrough/vtd/dmar.c | 201 +-- xen/include/xen/iommu.h | 13 +- xen/include/xen/serial.h |6 +- 12 files changed, 1690 insertions(+), 105 deletions(-) create mode 100644 xen/drivers/char/xhci-dbc.c base-commit: 4df2e99d731402da48afb19dc970ccab5a0814d6 -- git-series 0.9.1
[PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
From: Connor Davis
[Connor]
Xue is a cross-platform USB 3 debugger that drives the Debug
Capability (DbC) of xHCI-compliant host controllers. This patch
implements the operations needed for xue to initialize the host
controller's DbC and communicate with it. It also implements a struct
uart_driver that uses xue as a backend. Note that only target -> host
communication is supported for now. To use Xue as a console, add
'console=dbgp dbgp=xhci' to the command line.
[Marek]
The Xue driver is taken from https://github.com/connojd/xue and heavily
refactored to fit into Xen code base. Major changes include:
- rename to xhci_dbc
- drop support for non-Xen systems
- drop xue_ops abstraction
- use Xen's native helper functions for PCI access
- move all the code to xue.c, drop "inline"
- build for x86 only
- annotate functions with cf_check
- adjust for Xen's code style
At this stage, only the first xHCI is considered. Later patch adds
support for choosing specific one.
The driver is initiallized before memory allocator works, so all the
transfer buffers (about 230KiB of them) are allocated statically and will
use memory even if XUE console is not selected. The driver can be
disabled build time to reclaim this memory.
Signed-off-by: Connor Davis
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- rename to xhci-dbc
- add empty stub for xhci_dbc_uart_init(), to avoid #ifdef in setup.c
- use PCI_BASE_ADDRESS_MEM_MASK
- make strings init more readable
- avoid infinite extended caps lookup
- size the whole 64bit BAR
- rename CONFIG_HAS_XHCI to CONFIG_XHCI
- use cpu_relax(), drop xue_sys_pause()
- disable memory decoding for the BAR sizing time, and enable it
explicitly
- drop mmio_size field - it's used only in init_xhc() function
internally
- use readl()/writel() for accessing MMIO
- add pci_ro_device(), to protect device before later patch(es) add
other protections
- fix setting dequeue pointer based on events: TRB ring was page
aligned, not 16-page aligned, so just taking DBC_TRB_RING_MASK bits
doesn't work; instead, calculate distance from the ring beginning;
while at it, fix off by one error there; dequeue pointer isn't used
much yet, but it will be useful for RX handling
- split dbc_ensure_running() out of dbc_flush() - it make dbc_flush()
more logical, and will make even more sense with RX support added
- make enum names upper case
Changes in v2:
- drop #pragma pack
- fix indentation in Kconfig
- minor style fixes
- use cache_flush()
- mark init functions as __init, and return bool
- fix PCI_SBDF usage, use constants from pci_regs.h
- add command line docs
- allow disabling the driver in menuconfig, to reclaim 2MB allocated
memory
- guard unused debug functions with #ifdef XUE_DEBUG
---
docs/misc/xen-command-line.pandoc |5 +-
xen/arch/x86/include/asm/fixmap.h |4 +-
xen/arch/x86/setup.c |1 +-
xen/drivers/char/Kconfig |9 +-
xen/drivers/char/Makefile |1 +-
xen/drivers/char/xhci-dbc.c | 1024 ++-
xen/include/xen/serial.h |5 +-
7 files changed, 1049 insertions(+)
create mode 100644 xen/drivers/char/xhci-dbc.c
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index da18172e50c5..f936283cd187 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -721,10 +721,15 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ | @pci:. ]`
+> `= xhci`
Specify the USB controller to use, either by instance number (when going
over the PCI busses sequentially) or by PCI device (must be on segment 0).
+Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability (output
+only). XHCI driver will wait indefinitely for the debug host to connect - make
+sure the cable is connected.
+
### debug_stack_lines
> `= `
diff --git a/xen/arch/x86/include/asm/fixmap.h
b/xen/arch/x86/include/asm/fixmap.h
index 20746afd0a2a..bc39ffe896b1 100644
--- a/xen/arch/x86/include/asm/fixmap.h
+++ b/xen/arch/x86/include/asm/fixmap.h
@@ -25,6 +25,8 @@
#include
#include
+#define MAX_XHCI_PAGES 16
+
/*
* Here we define all the compile-time 'special' virtual
* addresses. The point is to have a constant address at
@@ -43,6 +45,8 @@ enum fixed_addresses {
FIX_COM_BEGIN,
FIX_COM_END,
FIX_EHCI_DBGP,
+FIX_XHCI_BEGIN,
+FIX_XHCI_END = FIX_XHCI_BEGIN + MAX_XHCI_PAGES - 1,
#ifdef CONFIG_XEN_GUEST
FIX_PV_CONSOLE,
FIX_XEN_SHARED_INFO,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8dea6..e05189f64997 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -950,6 +950,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
ns16550.irq = 3;
ns16550_init(1, );
ehci_dbgp_init();
+xhci_dbc_uart_init();
console_init_preirq();
if ( pvh_boot )
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index
RE: [PATCH v9 2/8] xen: do not free reserved memory into heap
Hi Jan
> -Original Message-
> From: Jan Beulich
> Sent: Monday, July 25, 2022 11:30 PM
> To: Penny Zheng
> Cc: Wei Chen ; Stefano Stabellini
> ; Julien Grall ; Bertrand Marquis
> ; Volodymyr Babchuk
> ; Andrew Cooper
> ; George Dunlap ;
> Wei Liu ; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v9 2/8] xen: do not free reserved memory into heap
>
> On 20.07.2022 07:46, Penny Zheng wrote:
> > Pages used as guest RAM for static domain, shall be reserved to this
> > domain only.
> > So in case reserved pages being used for other purpose, users shall
> > not free them back to heap, even when last ref gets dropped.
> >
> > This commit introduces a new helper free_domstatic_page to free static
> > page in runtime, and free_staticmem_pages will be called by it in
> > runtime, so let's drop the __init flag.
> >
> > Signed-off-by: Penny Zheng
>
> Technically
> Reviewed-by: Jan Beulich
>
> Nevertheless two remarks:
>
> > +void free_domstatic_page(struct page_info *page) {
> > +struct domain *d = page_get_owner(page);
> > +bool drop_dom_ref;
> > +
> > +ASSERT(d);
>
> I wonder whether
>
> if ( unlikely(!d) )
> {
> ASSERT_UNREACHABLE();
> return;
> }
>
> wouldn't be more robust looking forward.
>
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,13 +85,12 @@ bool scrub_free_pages(void); } while ( false )
> > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >
> > -#ifdef CONFIG_STATIC_MEMORY
> > /* These functions are for static memory */ void
> > free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >bool need_scrub);
> > +void free_domstatic_page(struct page_info *page);
> > int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> > unsigned int memflags); -#endif
> >
> > /* Map machine page range in Xen virtual address space. */ int
> > map_pages_to_xen( @@ -212,6 +211,10 @@ extern struct domain
> *dom_cow;
> >
> > #include
> >
> > +#ifndef PGC_static
> > +#define PGC_static 0
> > +#endif
>
> This disconnect from all other PGC_* values isn't very nice. I wonder as how
> bad it would be seen if Arm kept its #define to 0 private, with the generic
> fallback remaining in page_alloc.c.
>
It, right now, is only used in xen/arch/arm/mm.c and xen/common/page_alloc.c.
It is ok to let Arm keep its #define to 0 private, with the generic
fallback remaining in page_alloc.c.
> Jan
RE: [PATCH v9 6/8] xen/arm: unpopulate memory when domain is static
Hi Jan > -Original Message- > From: Jan Beulich > Sent: Monday, July 25, 2022 11:36 PM > To: Penny Zheng > Cc: Wei Chen ; Andrew Cooper > ; George Dunlap ; > Julien Grall ; Stefano Stabellini ; > Wei Liu ; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v9 6/8] xen/arm: unpopulate memory when domain is > static > > On 20.07.2022 07:46, Penny Zheng wrote: > > Today when a domain unpopulates the memory on runtime, they will > > always hand the memory back to the heap allocator. And it will be a > > problem if domain is static. > > > > Pages as guest RAM for static domain shall be reserved to only this > > domain and not be used for any other purposes, so they shall never go > > back to heap allocator. > > > > This commit puts reserved pages on the new list resv_page_list only > > after having taken them off the "normal" list, when the last ref dropped. > > I guess this wording somehow relates to ... > > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info > > *page) > > > > drop_dom_ref = !domain_adjust_tot_pages(d, -1); > > > > -spin_unlock_recursive(>page_alloc_lock); > > - > > free_staticmem_pages(page, 1, scrub_debug); > > > > +/* Add page on the resv_page_list *after* it has been freed. */ > > +if ( !drop_dom_ref ) > > +page_list_add_tail(page, >resv_page_list); > > ... the conditional used here. I cannot, however, figure why there is this > conditional (and said part of the description also doesn't help me figure it > out). > I was thinking that if drop_dom_ref true, then later the whole domain struct will be released, then there is no need to add the page to d->resv_page_list > As an aside: A title prefix of xen/arm: suggests you're mostly touching Arm > code. But you're touching exclusively common code here. > I for one would almost have skipped this patch (more than once) when > deciding which ones (may) want me looking at them. > Sorry for that, I’ll fix it > Jan
[ovmf test] 171858: all pass - PUSHED
flight 171858 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/171858/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 7f1c89f16790fc2fa8bc88330dc896941b9b40bb baseline version: ovmf a47241f1337c2ce78179b7db939faebd7828d8d0 Last test of basis 171855 2022-07-25 16:40:24 Z0 days Testing same since 171858 2022-07-25 23:40:21 Z0 days1 attempts People who touched revisions under test: Anthony PERARD Gary Lin Michael D Kinney jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git a47241f133..7f1c89f167 7f1c89f16790fc2fa8bc88330dc896941b9b40bb -> xen-tested-master
Re: Licensing issues
On Fri, 22 Jul 2022, Andrew Cooper wrote: > I'm also intending to start using SDPX identifiers to save on all the > boilerplate. They're already used elsewhere. I just wanted to add that adding/using SPDX is important and came up quite a few times in Linux Foundation discussions recently [1]. Linux and Zephyr already support it. As a community, I think we should move toward it reasonably quickly. [1] https://elisa.tech/blog/2022/06/29/elisas-new-systems-working-group/?utm_campaign=ELISA%20Project%20Blog_content=213223762_medium=social_source=linkedin_channel=lcp-35579852
Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
On Mon, 25 Jul 2022, Xenia Ragiadakou wrote:
> On 7/25/22 09:32, Jan Beulich wrote:
> > On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> > > On 7/7/22 10:55, Jan Beulich wrote:
> > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote:
> > > > > On 7/6/22 11:51, Jan Beulich wrote:
> > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> > > > > > > On 7/6/22 10:10, Jan Beulich wrote:
> > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote:
> > > > > > > > > The function idle_loop() is referenced only in domain.c.
> > > > > > > > > Change its linkage from external to internal by adding the
> > > > > > > > > storage-class
> > > > > > > > > specifier static to its definitions.
> > > > > > > > >
> > > > > > > > > Since idle_loop() is referenced only in inline assembly, add
> > > > > > > > > the 'used'
> > > > > > > > > attribute to suppress unused-function compiler warning.
> > > > > > > >
> > > > > > > > While I see that Julien has already acked the patch, I'd like to
> > > > > > > > point
> > > > > > > > out that using __used here is somewhat bogus. Imo the better
> > > > > > > > approach
> > > > > > > > is to properly make visible to the compiler that the symbol is
> > > > > > > > used by
> > > > > > > > the asm(), by adding a fake(?) input.
> > > > > > >
> > > > > > > I 'm afraid I do not understand what do you mean by "adding a
> > > > > > > fake(?)
> > > > > > > input". Can you please elaborate a little on your suggestion?
> > > > > >
> > > > > > Once the asm() in question takes the function as an input, the
> > > > > > compiler
> > > > > > will know that the function has a user (unless, of course, it finds
> > > > > > a
> > > > > > way to elide the asm() itself). The "fake(?)" was because I'm not
> > > > > > deeply
> > > > > > enough into Arm inline assembly to know whether the input could then
> > > > > > also be used as an instruction operand (which imo would be
> > > > > > preferable) -
> > > > > > if it can't (e.g. because there's no suitable constraint or operand
> > > > > > modifier), it still can be an input just to inform the compiler.
> > > > >
> > > > > According to the following statement, your approach is the recommended
> > > > > one:
> > > > > "To prevent the compiler from removing global data or functions which
> > > > > are referenced from inline assembly statements, you can:
> > > > > -use __attribute__((used)) with the global data or functions.
> > > > > -pass the reference to global data or functions as operands to inline
> > > > > assembly statements.
> > > > > Arm recommends passing the reference to global data or functions as
> > > > > operands to inline assembly statements so that if the final image does
> > > > > not require the inline assembly statements and the referenced global
> > > > > data or function, then they can be removed."
> > > > >
> > > > > IIUC, you are suggesting to change
> > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> > > > > into
> > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory"
> > > > > );
> > > >
> > > > Yes, except that I can't judge about the "S" constraint.
> > > >
> > >
> > > This constraint does not work for arm32. Any other thoughts?
> > >
> > > Another way, as Jan suggested, is to pass the function as a 'fake'
> > > (unused) input.
> > > I 'm suspecting something like the following could work
> > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) :
> > > "memory")
> > > What do you think?
> >
> > Well, yes, X should always be a fallback option. But I said already when
> > you suggested S that I can't judge about its use, so I guess I'm the
> > wrong one to ask. Even more so that you only say "does not work", without
> > any details ...
> >
>
> The question is addressed to anyone familiar with arm inline assembly.
> I added the arm maintainers as primary recipients to this email to make this
> perfectly clear.
>
> When cross-compiling Xen on x86 for arm32 with
> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
> compilation fails with the error: impossible constraint in ‘asm'
Unfortunately looking at the GCC manual pages [1], it doesn't seem to be
possible. The problem is that the definition of "S" changes between
aarch64 and arm (the 32-bit version).
For aarch64:
S An absolute symbolic address or a label reference
This is what we want. For arm instead:
S A symbol in the text segment of the current file
This is not useful for what we need to do here. As far as I can tell,
there is no other way in GCC assembly inline for arm to do this.
So we have 2 choices: we use the __used keyword as Xenia did in this
patch. Or we move the implementation of switch_stack_and_jump in
assembly. I attempted a prototype of the latter to see how it would come
out, see below.
I don't like it very much. I prefer the version with __used that Xenia
had in this patch. But I am OK either way.
diff --git a/xen/arch/arm/arm32/entry.S
[xen-unstable test] 171856: tolerable FAIL - PUSHED
flight 171856 xen-unstable real [real] flight 171857 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/171856/ http://logs.test-lab.xenproject.org/osstest/logs/171857/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-install fail pass in 171857-retest test-arm64-arm64-libvirt-raw 13 guest-start fail pass in 171857-retest Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 14 migrate-support-check fail in 171857 never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-check fail in 171857 never pass test-amd64-i386-libvirt 7 xen-install fail like 171848 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail like 171848 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 171848 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171848 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171848 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171848 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 171848 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 171848 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171848 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171848 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 171848 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171848 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171848 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171848 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-check
Re: Enable audio virtualization in Xen
On Mon, Jul 25, 2022 at 4:45 AM SHARMA, JYOTIRMOY
wrote:
>
> [AMD Official Use Only - General]
>
>
> Hi all,
Hi Jyotirmoy,
I have add the xen-users list to CC since this thread may be useful to
that forum.
> I am using ubuntu as dom 0 and also dom U (HVM). I want to play audio from
> “dom U” Ubuntu.
I think that it should be possible to enable what you are attempting
to do. ie. audio output from a HVM Ubuntu guest VM.
Some questions to support assisting you:
* Does audio playback work OK from your Ubuntu dom0?
* Do you know which version of Ubuntu you are using? ('cat /etc/lsb-release')
* Do you know which version of Xen you are using? ('xl info' in dom0
should help)
* Do you know which version of Qemu you have installed in dom0?
* Did you build and install Xen from source code, or are you using
binary packages of Xen and its tools?
* Are you using the xl tools, or libvirt tools for configuring and
running your guest? -- ie. how do you start your domU VM?
* When your domU is running, please could you run 'ps auxwww' in dom0
and obtain the process information about the qemu instance that is
running, so that we can see what command line arguments have been
supplied to it
> I am new to Xen/virtualization in general.
Welcome! :-)
> From various reading I understood that I need to take following approach:
>
> 1. Use Xen front end ALSA driver in dom U
I'm not certain that this is necessary for your HVM guest. Instead of
using the Xen paravirtualized audio protocol, Qemu should be able to
present an emulated audio device to the HVM guest domU, and a standard
audio driver (hda or ac97) in domU should suffice.
> 2. Use Qemu to connect to the backend ALSA driver in Dom 0
I think if Qemu is started with the correct command line arguments, it
should be able to play sound on behalf of the guest, if sound is
correctly configured and working in dom0.
> Can you please let me know if this approach is fine? If yes, I have following
> questions:
>
> 1. Do I need to recompile Ubuntu to support Xen front end ALSA driver? Or
> will Ubuntu iso file already have it enabled?
I think the latter, that the Ubuntu installation ISO should already
contain a suitable audio device driver that is compatible with the
virtual audio device that is emulated by Qemu.
> 2. Ho do I configure Qemu to enable backend driver?
A little more information about what you're running will help with
providing guidance here. The xl man page indicates that there is a
"soundhw" option in the VM configuration file for passing sound
hardware configution through to qemu, so if you are using the xl
toolstack, you could try adding this to the config file: soundhw="hda"
Christopher
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/25/22 6:03 AM, Jane Malalane wrote:
On 18/07/2022 14:59, Boris Ostrovsky wrote:
On 7/18/22 4:56 AM, Andrew Cooper wrote:
On 15/07/2022 14:10, Boris Ostrovsky wrote:
On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
On 14/07/2022 00:27, Boris Ostrovsky wrote:
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+
BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+ ));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a*
callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
Feels like it (setting the callback parameter) is something that the
hypervisor should do --- no need to expose guests to this.
Sensible or not, it is the ABI.
Linux still needs to work (nicely) with older Xen's in the world, and we
can't just retrofit a change in the hypervisor which says "btw, this ABI
we've just changed now has a side effect of modifying a field that you
also logically own".
The hypercall has been around for a while so I understand ABI concerns
there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
Why not tie presence of this bit to no longer having to explicitly set
the callback field?
Any other opinions on this?
(i.e., calling xen_set_callback_via(1) after
HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
recently added)
CPUID won't help here, I wasn't thinking clearly.
Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function that
will decide whether or not to also do xen_set_callback_via(1)?
-boris
Re: [PATCH] page-alloc: fix initialization of cross-node regions
On 25/07/2022 14:10, Jan Beulich wrote:
> Quite obviously to determine the split condition successive pages'
> attributes need to be evaluated, not always those of the initial page.
>
> Fixes: 72b02bc75b47 ("xen/heap: pass order to free_heap_pages() in heap init")
> Signed-off-by: Jan Beulich
> ---
> Part of the problem was already introduced in 24a53060bd37 ("xen/heap:
> Split init_heap_pages() in two"), but there it was still benign.
This also fixes the crash that XenRT found on loads of hardware, which
looks something like:
(XEN) NUMA: Allocated memnodemap from 105bc81000 - 105bc92000
(XEN) NUMA: Using 8 for the hash shift.
(XEN) Early fatal page fault at e008:82d04022ae1e
(cr2=00b8, ec=0002)
(XEN) [ Xen-4.17.0 x86_64 debug=y Not tainted ]
(XEN) CPU: 0
(XEN) RIP: e008:[]
common/page_alloc.c#free_heap_pages+0x2dd/0x850
...
(XEN) Xen call trace:
(XEN) [] R
common/page_alloc.c#free_heap_pages+0x2dd/0x850
(XEN) [] F
common/page_alloc.c#init_heap_pages+0x55f/0x720
(XEN) [] F end_boot_allocator+0x187/0x1e7
(XEN) [] F __start_xen+0x1a06/0x2779
(XEN) [] F __high_start+0x94/0xa0
Debugging shows that it's always a block which crosses node 0 and 1,
where avail[1] has yet to be initialised.
What I'm confused by is how this manages to manifest broken swiotlb
issues without Xen crashing.
~Andrew
[ovmf test] 171855: all pass - PUSHED
flight 171855 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/171855/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf a47241f1337c2ce78179b7db939faebd7828d8d0 baseline version: ovmf 8a5782d704cfeb78aafdec1c03685107586f4ee6 Last test of basis 171849 2022-07-25 02:42:04 Z0 days Testing same since 171855 2022-07-25 16:40:24 Z0 days1 attempts People who touched revisions under test: PaytonX Hsieh jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 8a5782d704..a47241f133 a47241f1337c2ce78179b7db939faebd7828d8d0 -> xen-tested-master
[PATCH] x86/pv: Inject #GP for implicit grant unmaps
This is a debug behaviour to identify buggy kernels. Crashing the domain is the most unhelpful thing to do, because it discards the relevant context. Instead, inject #GP[0] like other permission errors in x86. In particular, this lets the kernel provide a backtrace that's actually helpful to a developer trying to figure out what's going wrong. As a bugfix, this always injects #GP[0] to current, not l1e_owner. It is not l1e_owner's fault if dom0 using superpowers triggers an implicit unmap. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu CC: Juergen Gross This is a prerequisite to investigating https://github.com/QubesOS/qubes-issues/issues/7631 which is looking like an error in Linux's gntdev driver. --- xen/arch/x86/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 5b81d5fbdbb2..b3393385ffb6 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1232,7 +1232,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner) gdprintk(XENLOG_WARNING, "Attempt to implicitly unmap a granted PTE %" PRIpte "\n", l1e_get_intpte(l1e)); -domain_crash(l1e_owner); +pv_inject_hw_exception(TRAP_gp_fault, 0); } #endif -- 2.11.0
[IMAGEBUILDER PATCH] uboot-script-gen: allow fit generation with no dom0 kernel
Hi Stefano,
My colleague Jason Lei and I would like to submit a patch to imagebuilder.
It seems that generating a .fit with a true dom0less configuration fails
because an extraneous comma is included in the its file.
We believe this change resolves the issue.
Thanks,
Jackson
-- >8 --
Remove extraneous comma in generated its file when no DOM0_KERNEL is specified.
diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index 8f08cd6..6f94fce 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -676,7 +676,12 @@ create_its_file_xen()
i=$(( $i + 1 ))
continue
fi
-load_files+=", \"domU${i}_kernel\""
+ if test -z "$load_files"
+ then
+ load_files+="\"domU${i}_kernel\""
+ else
+ load_files+=", \"domU${i}_kernel\""
+ fi
cat >> "$its_file" <<- EOF
domU${i}_kernel {
description = "domU${i} kernel binary";
[PATCH v3] livepatch: create-diff-object: Check that the section has a secsym
A STT_SECTION symbol is not needed if if it is not used as a relocation
target. Therefore, a section, in this case a debug section, may not have
a secsym associated with it.
Signed-off-by: Bill Wendling
Origin: https://github.com/dynup/kpatch.git ba3defa06073
Signed-off-by: Sarah Newman
Reviewed-by: Ross Lagerwall
---
Changes in v3:
- add reviewed-by given to v1 of this patch
- restored tag from original commit per sending-patches.pandoc
Changes in v2:
- commit message changed to use Origin
---
create-diff-object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/create-diff-object.c b/create-diff-object.c
index a516670..780e6c8 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1484,7 +1484,7 @@ static void kpatch_include_debug_sections(struct
kpatch_elf *kelf)
list_for_each_entry(sec, >sections, list) {
if (is_debug_section(sec)) {
sec->include = 1;
- if (!is_rela_section(sec))
+ if (!is_rela_section(sec) && sec->secsym)
sec->secsym->include = 1;
}
}
--
2.17.1
[xen-unstable-smoke test] 171854: tolerable all pass - PUSHED
flight 171854 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/171854/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen f1c719d5cd8ab4d5d4c8df139b9df3c2c47221d1 baseline version: xen fcd27b3c759995775afb66be6bb7ba1e85da0506 Last test of basis 171747 2022-07-21 20:05:50 Z3 days Testing same since 171854 2022-07-25 14:03:15 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Oleksandr Tyshchenko Roger Pau Monné Tamas K Lengyel jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git fcd27b3c75..f1c719d5cd f1c719d5cd8ab4d5d4c8df139b9df3c2c47221d1 -> smoke
Re: [PATCH] page-alloc: fix initialization of cross-node regions
On 25.07.2022 18:05, Julien Grall wrote:
> (Sorry for the formatting)
No issues seen.
> On Mon, 25 Jul 2022, 14:10 Jan Beulich, wrote:
>
>> Quite obviously to determine the split condition successive pages'
>> attributes need to be evaluated, not always those of the initial page.
>>
>> Fixes: 72b02bc75b47 ("xen/heap: pass order to free_heap_pages() in heap
>> init")
>> Signed-off-by: Jan Beulich
>> ---
>> Part of the problem was already introduced in 24a53060bd37 ("xen/heap:
>> Split init_heap_pages() in two"), but there it was still benign.
>>
>
> Is this because range will never cross numa node? How about the fake NUMA
> node?
No (afaict), because pages were still freed one by one (and hence node
boundaries still wouldn't end up in the middle of a buddy).
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1885,11 +1885,11 @@ static void init_heap_pages(
>> * range to cross zones.
>> */
>> #ifdef CONFIG_SEPARATE_XENHEAP
>> -if ( zone != page_to_zone(pg) )
>> +if ( zone != page_to_zone(pg + contig_pages) )
>> break;
>> #endif
>>
>> -if ( nid != (phys_to_nid(page_to_maddr(pg))) )
>> +if ( nid != (phys_to_nid(page_to_maddr(pg + contig_pages))) )
>> break;
>> }
>>
>
> Hmmm I am not sure why I didn't spot this issue during my testing. It looks
> like this was introduced in v2, sorry for that.
>
> Reviewed-by: Julien Grall
Thanks.
Jan
Re: Random crash during guest start on x86
On 25.07.2022 17:51, Bertrand Marquis wrote: > On our CI we have randomly a crash during guest boot on x86. Afaict of a PV guest. > We are running on qemu x86_64 using Xen staging. Which may introduce unusual timing. An issue never hit on actual hardware _may_ (but doesn't have to be) one in qemu itself. > The crash is happening randomly (something like 1 out of 20 times). > > This is always happening on the first guest we start, we never got it after > first guest was successfully started. > > Please tell me if you need any other info. > > Here is the guest kernel log: >[...] > [6.679020] general protection fault, maybe for address 0x8800: [#1] > PREEMPT SMP NOPTI > [6.679020] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.17.6 #1 > [6.679020] RIP: e030:error_entry+0xaf/0xe0 > [6.679020] Code: 29 89 c8 48 39 84 24 88 00 00 00 74 15 48 81 bc 24 88 00 > 00 00 63 10 e0 81 75 03 0f 01 f8 90 90 90 c3 48 89 8c 24 88 00 00 00 <0f> 01 > f8 90 90 90 eb 11 0f 20 d8 90 90 90 90 90 48 25 ff e7 ff ff This is SWAPGS, which supposedly a PV guest should never hit. Data further down suggests the kernel is still in the process of patching alternatives, which may be the reason for the insn to still be there (being at a point where exceptions are still unexpected). > [6.679020] RSP: e02b:82803a90 EFLAGS: 0046 > [6.679020] RAX: 8800 RBX: RCX: > 81e00fa7 > [6.679020] RDX: RSI: 81e009f8 RDI: > 00eb > [6.679020] RBP: R08: R09: > > [6.679020] R10: R11: R12: > > [6.679020] R13: R14: R15: > > [6.679020] FS: () GS:88801f20() > knlGS: > [6.679020] CS: 1e030 DS: ES: CR0: 80050033 > [6.679020] CR2: CR3: 0280c000 CR4: > 00050660 > [6.679020] Call Trace: > [6.679020] > [6.679020] RIP: e030:native_irq_return_iret+0x0/0x2 > [6.679020] Code: 41 5d 41 5c 5d 5b 41 5b 41 5a 41 59 41 58 58 59 5a 5e 5f > 48 83 c4 08 eb 0a 0f 1f 00 90 66 0f 1f 44 00 00 f6 44 24 20 04 75 02 <48> cf > 57 0f 01 f8 eb 12 0f 20 df 90 90 90 90 90 48 81 e7 ff e7 ff > [6.679020] RSP: e02b:82803b48 EFLAGS: 0046 ORIG_RAX: > e030 > [6.679020] RAX: 8800 RBX: 82803be0 RCX: > 81e00f95 > [6.679020] RDX: 81e00f94 RSI: 81e00f95 RDI: > 00eb > [6.679020] RBP: 00eb R08: 90001f0f R09: > 0007 > [6.679020] R10: 81e00f94 R11: 8285a6c0 R12: > > [6.679020] R13: 81e00f94 R14: 0006 R15: > 0006 > [6.679020] ? asm_exc_general_protection+0x8/0x30 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] ? restore_regs_and_return_to_kernel+0x1c/0x27 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] ? restore_regs_and_return_to_kernel+0x1c/0x27 > [6.679020] RIP: e030:insn_get_opcode.part.0+0xab/0x180 > [6.679020] Code: 00 00 8b 43 4c a9 c0 07 08 00 0f 84 bf 00 00 00 c6 43 1c > 01 31 c0 5b 5d c3 83 e2 03 be 01 00 00 00 eb b7 89 ef e8 65 e4 ff ff <89> 43 > 4c a8 30 75 21 e9 8e 00 00 00 0f b6 7b 03 40 84 ff 75 73 8b > [6.679020] RSP: e02b:82803b70 EFLAGS: 0246 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] insn_get_modrm+0x6c/0x120 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] insn_get_sib+0x40/0x80 > [6.679020] insn_get_displacement+0x82/0x100 > [6.679020] insn_decode+0xf8/0x100 > [6.679020] optimize_nops+0x60/0x1e0 > [6.679020] ? rcu_nmi_exit+0x2b/0x140 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] ? native_iret+0x3/0x7 > [6.679020] ? restore_regs_and_return_to_kernel+0x1c/0x27 > [6.679020] ? restore_regs_and_return_to_kernel+0x1b/0x27 > [6.679020] apply_alternatives+0x165/0x2e0 I have to admit that I'm a little lost with these "modern" stack traces, where contexts apparently switch without being clearly annotated. It is looking a little as if a #GP fault was happening somewhere here (hence the asm_exc_general_protection further up), but I cannot work out where (what insn) that would have come from. You may want to add some debugging code to the hypervisor to tell you where exactly that #GP (if there is one in the first place) originates from. With that it may then become a little more clear what's actually going on (and why the behavior is random). As a final remark - you've Cc-ed the x86 hypervisor maintainers, but at least from the data which is available so far this is
Re: [PATCH] page-alloc: fix initialization of cross-node regions
Hi Jan,
(Sorry for the formatting)
On Mon, 25 Jul 2022, 14:10 Jan Beulich, wrote:
> Quite obviously to determine the split condition successive pages'
> attributes need to be evaluated, not always those of the initial page.
>
> Fixes: 72b02bc75b47 ("xen/heap: pass order to free_heap_pages() in heap
> init")
> Signed-off-by: Jan Beulich
> ---
> Part of the problem was already introduced in 24a53060bd37 ("xen/heap:
> Split init_heap_pages() in two"), but there it was still benign.
>
Is this because range will never cross numa node? How about the fake NUMA
node?
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1885,11 +1885,11 @@ static void init_heap_pages(
> * range to cross zones.
> */
> #ifdef CONFIG_SEPARATE_XENHEAP
> -if ( zone != page_to_zone(pg) )
> +if ( zone != page_to_zone(pg + contig_pages) )
> break;
> #endif
>
> -if ( nid != (phys_to_nid(page_to_maddr(pg))) )
> +if ( nid != (phys_to_nid(page_to_maddr(pg + contig_pages))) )
> break;
> }
>
Hmmm I am not sure why I didn't spot this issue during my testing. It looks
like this was introduced in v2, sorry for that.
Reviewed-by: Julien Grall
Cheets,
>
Re: Random crash during guest start on x86
On 25/07/2022 16:51, Bertrand Marquis wrote: > Hi, > > On our CI we have randomly a crash during guest boot on x86. > > We are running on qemu x86_64 using Xen staging. > The crash is happening randomly (something like 1 out of 20 times). > > This is always happening on the first guest we start, we never got it after > first guest was successfully started. > > Please tell me if you need any other info. > > Here is the guest kernel log: > > [0.00] Hypervisor detected: Xen PV > > [6.679020] general protection fault, maybe for address 0x8800: [#1] > PREEMPT SMP NOPTI > [6.679020] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.17.6 #1 > [6.679020] RIP: e030:error_entry+0xaf/0xe0 > [6.679020] Code: 29 89 c8 48 39 84 24 88 00 00 00 74 15 48 81 bc 24 88 00 > 00 00 63 10 e0 81 75 03 0f 01 f8 90 90 90 c3 48 89 8c 24 88 00 00 00 <0f> 01 > f8 90 90 90 eb 11 0f 20 d8 90 90 90 90 90 48 25 ff e7 ff ff > [6.679020] RSP: e02b:82803a90 EFLAGS: 0046 > [6.679020] RAX: 8800 RBX: RCX: > 81e00fa7 > [6.679020] RDX: RSI: 81e009f8 RDI: > 00eb > [6.679020] RBP: R08: R09: > > [6.679020] R10: R11: R12: > > [6.679020] R13: R14: R15: > > [6.679020] FS: () GS:88801f20() > knlGS: > [6.679020] CS: 1e030 DS: ES: CR0: 80050033 > [6.679020] CR2: CR3: 0280c000 CR4: > 00050660 > [6.679020] Call Trace: > [6.679020] > 0f 01 f8 is SWAPGS This is a privileged instruction, and has never been permitted under Xen PV. This should have been excluded by pvops. This is a Linux bug, not a Xen bug. I can't explain why you're only seeing it intermittently. Perhaps error_entry is broken by default, and pvops fixes things up, but an error early enough takes a non-fixed-up path :-/ ~Andrew
Random crash during guest start on x86
Hi, On our CI we have randomly a crash during guest boot on x86. We are running on qemu x86_64 using Xen staging. The crash is happening randomly (something like 1 out of 20 times). This is always happening on the first guest we start, we never got it after first guest was successfully started. Please tell me if you need any other info. Here is the guest kernel log: [0.00] Linux version 5.17.6 (@ip-100-64-206-234.eu-west-1.compute.internal) (gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #1 SMP PREEMPT Mon Jul 25 14:54:50 UTC 2022 [0.00] Command line: console=hvc0 rdinit=/sbin/init [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers' [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' [0.00] x86/fpu: Enabled xstate features 0x3, context size is 576 bytes, using 'standard' format. [0.00] signal: max sigframe size: 1520 [0.00] ACPI in unprivileged domain disabled [0.00] Released 0 page(s) [0.00] BIOS-provided physical RAM map: [0.00] Xen: [mem 0x-0x0009] usable [0.00] Xen: [mem 0x000a-0x000f] reserved [0.00] Xen: [mem 0x0010-0x1fff] usable [0.00] NX (Execute Disable) protection: active [0.00] DMI not present or invalid. [0.00] Hypervisor detected: Xen PV [4.842670] tsc: Fast TSC calibration failed [4.842916] tsc: Detected 2799.922 MHz processor [4.845741] last_pfn = 0x2 max_arch_pfn = 0x4 [4.845871] Disabled [4.845953] x86/PAT: MTRRs disabled, skipping PAT initialization too. [4.846227] x86/PAT: Configuration [0-7]: WB WT UC- UC WC WP UC UC [4.848955] Kernel/User page tables isolation: disabled on XEN PV. [5.214349] RAMDISK: [mem 0x0340-0x04158fff] [5.221796] NUMA turned off [5.221926] Faking a node at [mem 0x-0x1fff] [5.222712] NODE_DATA(0) allocated [mem 0x1ff17000-0x1ff1afff] [5.374020] Zone ranges: [5.374146] DMA [mem 0x1000-0x00ff] [5.374349] DMA32[mem 0x0100-0x1fff] [5.374450] Normal empty [5.374562] Movable zone start for each node [5.374647] Early memory node ranges [5.374730] node 0: [mem 0x1000-0x0009] [5.376184] node 0: [mem 0x0010-0x1fff] [5.376586] Initmem setup node 0 [mem 0x1000-0x1fff] [5.377581] On node 0, zone DMA: 1 pages in unavailable ranges [5.378034] On node 0, zone DMA: 96 pages in unavailable ranges [5.385508] p2m virtual area at (ptrval), size is 4000 [6.156878] Remapped 0 page(s) [6.160205] smpboot: Allowing 1 CPUs, 0 hotplug CPUs [6.161126] PM: hibernation: Registered nosave memory: [mem 0x-0x0fff] [6.161230] PM: hibernation: Registered nosave memory: [mem 0x000a-0x000f] [6.161425] [mem 0x2000-0x] available for PCI devices [6.161565] Booting kernel on Xen [6.161636] Xen version: 4.17-unstable (preserve-AD) [6.162085] clocksource: refined-jiffies: mask: 0x max_cycles: 0x, max_idle_ns: 1910969940391419 ns [6.179113] setup_percpu: NR_CPUS:64 nr_cpumask_bits:64 nr_cpu_ids:1 nr_node_ids:1 [6.200627] percpu: Embedded 53 pages/cpu s179864 r8192 d29032 u2097152 [6.204411] Fallback order for Node 0: 0 [6.204883] Built 1 zonelists, mobility grouping on. Total pages: 128768 [6.204954] Policy zone: DMA32 [6.205071] Kernel command line: console=hvc0 rdinit=/sbin/init [6.211967] Dentry cache hash table entries: 65536 (order: 7, 524288 bytes, linear) [6.214335] Inode-cache hash table entries: 32768 (order: 6, 262144 bytes, linear) [6.220677] mem auto-init: stack:off, heap alloc:off, heap free:off [6.242643] Memory: 463112K/523900K available (16396K kernel code, 2697K rwdata, 5140K rodata, 1380K init, 1032K bss, 60536K reserved, 0K cma-reserved) [6.250830] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 [6.292059] Dynamic Preempt: voluntary [6.296486] rcu: Preemptible hierarchical RCU implementation. [6.296556] rcu: RCU event tracing is enabled. [6.296636] rcu: RCU restricting CPUs from NR_CPUS=64 to nr_cpu_ids=1. [6.296805] Trampoline variant of Tasks RCU enabled. [6.296954] rcu: RCU calculated value of scheduler-enlistment delay is 100 jiffies. [6.297045] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1 [6.538636] Using NULL legacy PIC [6.538760] NR_IRQS: 4352, nr_irqs: 32, preallocated irqs: 0 [6.542021] xen:events: Using FIFO-based ABI [6.560876] random: get_random_bytes called from start_kernel+0x4a3/0x680 with crng_init=0 [6.573619] Console: colour dummy device 80x25 [6.575908] printk: console [tty0] enabled [
Re: [PATCH v2 3/5] xen: Rename CONFIG_DOMAIN_PAGE to CONFIG_ARCH_MAP_DOMAIN_PAGE and...
On 20.07.2022 20:44, Julien Grall wrote: > From: Julien Grall > > move it to Kconfig. > > The define CONFIG_DOMAIN_PAGE indicates whether the architecture provide > helpers to map/unmap a domain page. Rename it to the define to > CONFIG_ARCH_MAP_DOMAIN_PAGE so it is clearer that this will not remove > support for domain page (this is not a concept that Xen can't get > away with). Especially the part in parentheses reads odd, if not backwards. > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -371,7 +371,7 @@ void clear_fixmap(unsigned int map) > BUG_ON(res != 0); > } > > -#ifdef CONFIG_DOMAIN_PAGE > +#ifdef CONFIG_ARCH_MAP_DOMAIN_PAGE > /* > * Prepare the area that will be used to map domheap pages. They are > * mapped in 2MB chunks, so we need to allocate the page-tables up to What about the other #ifdef in build_assertions()? With that also converted (and preferably with the description adjusted) Reviewed-by: Jan Beulich Jan
Re: [PATCH v9 8/8] xen: retrieve reserved pages on populate_physmap
On 20.07.2022 07:46, Penny Zheng wrote:
> When a static domain populates memory through populate_physmap at runtime,
> it shall retrieve reserved pages from resv_page_list to make sure that
> guest RAM is still restricted in statically configured memory regions.
> This commit also introduces a new helper acquire_reserved_page to make it
> work.
>
> Signed-off-by: Penny Zheng
> ---
> v9 changes:
> - Use ASSERT_ALLOC_CONTEXT() in acquire_reserved_page
> - Add free_staticmem_pages to undo prepare_staticmem_pages when
> assign_domstatic_pages fails
May I suggest to re-consider naming of the various functions? Undoing
what "prepare" did by "free" is, well, counterintuitive.
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +struct page_info *page;
> +
> +ASSERT_ALLOC_CONTEXT();
> +
> +/* Acquire a page from reserved page list(resv_page_list). */
> +spin_lock(>page_alloc_lock);
> +page = page_list_remove_head(>resv_page_list);
> +spin_unlock(>page_alloc_lock);
> +if ( unlikely(!page) )
> +return INVALID_MFN;
> +
> +if ( !prepare_staticmem_pages(page, 1, memflags) )
> +goto fail;
> +
> +if ( assign_domstatic_pages(d, page, 1, memflags) )
> +goto fail_assign;
> +
> +return page_to_mfn(page);
> +
> + fail_assign:
> +free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> + fail:
> +page_list_add_tail(page, >resv_page_list);
> +return INVALID_MFN;
> +}
What about locking on this error path?
Jan
Re: [PATCH v9 6/8] xen/arm: unpopulate memory when domain is static
On 20.07.2022 07:46, Penny Zheng wrote: > Today when a domain unpopulates the memory on runtime, they will always > hand the memory back to the heap allocator. And it will be a problem if domain > is static. > > Pages as guest RAM for static domain shall be reserved to only this domain > and not be used for any other purposes, so they shall never go back to heap > allocator. > > This commit puts reserved pages on the new list resv_page_list only after > having taken them off the "normal" list, when the last ref dropped. I guess this wording somehow relates to ... > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info *page) > > drop_dom_ref = !domain_adjust_tot_pages(d, -1); > > -spin_unlock_recursive(>page_alloc_lock); > - > free_staticmem_pages(page, 1, scrub_debug); > > +/* Add page on the resv_page_list *after* it has been freed. */ > +if ( !drop_dom_ref ) > +page_list_add_tail(page, >resv_page_list); ... the conditional used here. I cannot, however, figure why there is this conditional (and said part of the description also doesn't help me figure it out). As an aside: A title prefix of xen/arm: suggests you're mostly touching Arm code. But you're touching exclusively common code here. I for one would almost have skipped this patch (more than once) when deciding which ones (may) want me looking at them. Jan
Re: [PATCH v9 2/8] xen: do not free reserved memory into heap
On 20.07.2022 07:46, Penny Zheng wrote:
> Pages used as guest RAM for static domain, shall be reserved to this
> domain only.
> So in case reserved pages being used for other purpose, users
> shall not free them back to heap, even when last ref gets dropped.
>
> This commit introduces a new helper free_domstatic_page to free
> static page in runtime, and free_staticmem_pages will be called by it
> in runtime, so let's drop the __init flag.
>
> Signed-off-by: Penny Zheng
Technically
Reviewed-by: Jan Beulich
Nevertheless two remarks:
> +void free_domstatic_page(struct page_info *page)
> +{
> +struct domain *d = page_get_owner(page);
> +bool drop_dom_ref;
> +
> +ASSERT(d);
I wonder whether
if ( unlikely(!d) )
{
ASSERT_UNREACHABLE();
return;
}
wouldn't be more robust looking forward.
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,13 +85,12 @@ bool scrub_free_pages(void);
> } while ( false )
> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>
> -#ifdef CONFIG_STATIC_MEMORY
> /* These functions are for static memory */
> void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>bool need_scrub);
> +void free_domstatic_page(struct page_info *page);
> int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> unsigned int memflags);
> -#endif
>
> /* Map machine page range in Xen virtual address space. */
> int map_pages_to_xen(
> @@ -212,6 +211,10 @@ extern struct domain *dom_cow;
>
> #include
>
> +#ifndef PGC_static
> +#define PGC_static 0
> +#endif
This disconnect from all other PGC_* values isn't very nice. I wonder
as how bad it would be seen if Arm kept its #define to 0 private, with
the generic fallback remaining in page_alloc.c.
Jan
Re: [PATCH] common/memory: Fix ifdefs for ptdom_max_order
On 25.07.2022 16:44, Luca Fancellu wrote:
> In common/memory.c the ifdef code surrounding ptdom_max_order is
> using HAS_PASSTHROUGH instead of CONFIG_HAS_PASSTHROUGH, fix the
> problem using the correct macro.
>
> Fixes: e0d44c1f9461 ("build: convert HAS_PASSTHROUGH use to Kconfig")
> Signed-off-by: Luca Fancellu
Reviewed-by: Jan Beulich
[PATCH] arm/domain: fix comment for arch_set_info_guest
The function arch_set_info_guest is not reached anymore through VCPUOP_initialise on arm, update the comment. Signed-off-by: Luca Fancellu --- xen/arch/arm/domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2f8eaab7b56b..6451cd013c1a 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -882,9 +882,9 @@ static int is_guest_pv64_psr(uint64_t psr) #endif /* - * Initialise VCPU state. The context can be supplied by either the - * toolstack (XEN_DOMCTL_setvcpucontext) or the guest - * (VCPUOP_initialise) and therefore must be properly validated. + * Initialise VCPU state. The context can be supplied by the toolstack + * (XEN_DOMCTL_setvcpucontext) and therefore must be properly validated, + * or by PSCI call (PSCI_cpu_on) handled by vpsci module. */ int arch_set_info_guest( struct vcpu *v, vcpu_guest_context_u c) -- 2.17.1
[PATCH] common/memory: Fix ifdefs for ptdom_max_order
In common/memory.c the ifdef code surrounding ptdom_max_order is
using HAS_PASSTHROUGH instead of CONFIG_HAS_PASSTHROUGH, fix the
problem using the correct macro.
Fixes: e0d44c1f9461 ("build: convert HAS_PASSTHROUGH use to Kconfig")
Signed-off-by: Luca Fancellu
---
xen/common/memory.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/common/memory.c b/xen/common/memory.c
index f6f794914dfd..bc89442ba53f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -58,7 +58,7 @@ struct memop_args {
static unsigned int __read_mostly domu_max_order = CONFIG_DOMU_MAX_ORDER;
static unsigned int __read_mostly ctldom_max_order = CONFIG_CTLDOM_MAX_ORDER;
static unsigned int __read_mostly hwdom_max_order = CONFIG_HWDOM_MAX_ORDER;
-#ifdef HAS_PASSTHROUGH
+#ifdef CONFIG_HAS_PASSTHROUGH
static unsigned int __read_mostly ptdom_max_order = CONFIG_PTDOM_MAX_ORDER;
#endif
@@ -70,7 +70,7 @@ static int __init cf_check parse_max_order(const char *s)
ctldom_max_order = simple_strtoul(s, , 0);
if ( *s == ',' && *++s != ',' )
hwdom_max_order = simple_strtoul(s, , 0);
-#ifdef HAS_PASSTHROUGH
+#ifdef CONFIG_HAS_PASSTHROUGH
if ( *s == ',' && *++s != ',' )
ptdom_max_order = simple_strtoul(s, , 0);
#endif
@@ -83,7 +83,7 @@ static unsigned int max_order(const struct domain *d)
{
unsigned int order = domu_max_order;
-#ifdef HAS_PASSTHROUGH
+#ifdef CONFIG_HAS_PASSTHROUGH
if ( cache_flush_permitted(d) && order < ptdom_max_order )
order = ptdom_max_order;
#endif
--
2.17.1
[PATCH] page-alloc: fix initialization of cross-node regions
Quite obviously to determine the split condition successive pages'
attributes need to be evaluated, not always those of the initial page.
Fixes: 72b02bc75b47 ("xen/heap: pass order to free_heap_pages() in heap init")
Signed-off-by: Jan Beulich
---
Part of the problem was already introduced in 24a53060bd37 ("xen/heap:
Split init_heap_pages() in two"), but there it was still benign.
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1885,11 +1885,11 @@ static void init_heap_pages(
* range to cross zones.
*/
#ifdef CONFIG_SEPARATE_XENHEAP
-if ( zone != page_to_zone(pg) )
+if ( zone != page_to_zone(pg + contig_pages) )
break;
#endif
-if ( nid != (phys_to_nid(page_to_maddr(pg))) )
+if ( nid != (phys_to_nid(page_to_maddr(pg + contig_pages))) )
break;
}
Re: [PATCH] include: correct re-building conditions around hypercall-defs.h
On 25.07.2022 14:24, Anthony PERARD wrote:
> On Mon, Jul 25, 2022 at 02:08:04PM +0200, Jan Beulich wrote:
>> For a .cmd file to be picked up, the respective target needs to be
>> listed in $(targets). This wasn't the case for hypercall-defs.i, leading
>> to permanent re-building even on an entirely unchanged tree (because of
>> the command apparently having changed).
>>
>> Fixes: eca1f00d0227 ("xen: generate hypercall interface related code")
>> Signed-off-by: Jan Beulich
>>
>> --- a/xen/include/Makefile
>> +++ b/xen/include/Makefile
>> @@ -114,7 +114,7 @@ all: $(obj)/xen/hypercall-defs.h
>> $(obj)/xen/hypercall-defs.h: $(obj)/hypercall-defs.i
>> $(srctree)/scripts/gen_hypercall.awk FORCE
>> $(call if_changed,genhyp)
>>
>> -targets += xen/hypercall-defs.h
>> +targets += hypercall-defs.i xen/hypercall-defs.h
>
> Do you want to remove "hypercall-defs.i" from $(clean-files) at the same
> time?
Oh, right - I certainly should.
> In any case,
> Reviewed-by: Anthony PERARD
Thanks.
Jan
Re: [PATCH] x86: limit issuing of IBPB during context switch
On 25.07.2022 14:15, Andrew Cooper wrote: > On 25/07/2022 13:09, Jan Beulich wrote: >> When the outgoing vCPU had IBPB issued upon entering Xen there's no >> need for a 2nd barrier during context switch. >> >> Signed-off-by: Jan Beulich > > That's already accounted for by opt_ibpb_ctxt_switch conditionally being > not set. That option defaults to false only if both PV and HVM have the entry barrier turned on. In fact, if it wasn't for Dom0 I was first thinking the global could go away and only the per-domain setting could be inspected here. Jan
Re: [PATCH] include: correct re-building conditions around hypercall-defs.h
On Mon, Jul 25, 2022 at 02:08:04PM +0200, Jan Beulich wrote:
> For a .cmd file to be picked up, the respective target needs to be
> listed in $(targets). This wasn't the case for hypercall-defs.i, leading
> to permanent re-building even on an entirely unchanged tree (because of
> the command apparently having changed).
>
> Fixes: eca1f00d0227 ("xen: generate hypercall interface related code")
> Signed-off-by: Jan Beulich
>
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -114,7 +114,7 @@ all: $(obj)/xen/hypercall-defs.h
> $(obj)/xen/hypercall-defs.h: $(obj)/hypercall-defs.i
> $(srctree)/scripts/gen_hypercall.awk FORCE
> $(call if_changed,genhyp)
>
> -targets += xen/hypercall-defs.h
> +targets += hypercall-defs.i xen/hypercall-defs.h
Do you want to remove "hypercall-defs.i" from $(clean-files) at the same
time?
In any case,
Reviewed-by: Anthony PERARD
Thanks,
--
Anthony PERARD
Re: [PATCH] x86: limit issuing of IBPB during context switch
On 25/07/2022 13:09, Jan Beulich wrote: > When the outgoing vCPU had IBPB issued upon entering Xen there's no > need for a 2nd barrier during context switch. > > Signed-off-by: Jan Beulich That's already accounted for by opt_ibpb_ctxt_switch conditionally being not set. ~Andrew
[PATCH] x86: limit issuing of IBPB during context switch
When the outgoing vCPU had IBPB issued upon entering Xen there's no
need for a 2nd barrier during context switch.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2098,7 +2098,8 @@ void context_switch(struct vcpu *prev, s
ctxt_switch_levelling(next);
-if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) )
+if ( opt_ibpb_ctxt_switch && !is_idle_domain(nextd) &&
+ !(prevd->arch.spec_ctrl_flags & SCF_entry_ibpb) )
{
static DEFINE_PER_CPU(unsigned int, last);
unsigned int *last_id = _cpu(last);
[PATCH] include: correct re-building conditions around hypercall-defs.h
For a .cmd file to be picked up, the respective target needs to be
listed in $(targets). This wasn't the case for hypercall-defs.i, leading
to permanent re-building even on an entirely unchanged tree (because of
the command apparently having changed).
Fixes: eca1f00d0227 ("xen: generate hypercall interface related code")
Signed-off-by: Jan Beulich
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -114,7 +114,7 @@ all: $(obj)/xen/hypercall-defs.h
$(obj)/xen/hypercall-defs.h: $(obj)/hypercall-defs.i
$(srctree)/scripts/gen_hypercall.awk FORCE
$(call if_changed,genhyp)
-targets += xen/hypercall-defs.h
+targets += hypercall-defs.i xen/hypercall-defs.h
ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
Enable audio virtualization in Xen
[AMD Official Use Only - General] Hi all, I am using ubuntu as dom 0 and also dom U (HVM). I want to play audio from "dom U" Ubuntu. I am new to Xen/virtualization in general. >From various reading I understood that I need to take following approach: 1. Use Xen front end ALSA driver in dom U 2. Use Qemu to connect to the backend ALSA driver in Dom 0 Can you please let me know if this approach is fine? If yes, I have following questions: 1. Do I need to recompile Ubuntu to support Xen front end ALSA driver? Or will Ubuntu iso file already have it enabled? 2. Ho do I configure Qemu to enable backend driver? Regards, Jyotirmoy
Re: [PATCH] Arm32: restore proper name of .dtb section start symbol
Hi Jan,
> On 25 Jul 2022, at 11:12, Jan Beulich wrote:
>
> This addresses a build failure when CONFIG_DTB_FILE evaluates to a non-
> empty string.
>
> Fixes: d07358f2dccd ("xen/arm32: head.S: Introduce a macro to load the
> physical address of a symbol")
> Signed-off-by: Jan Beulich
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
> ---
> Of course this really would be a prime candidate for avoiding the
> use of linker-script-defined symbols in the first place, by using
> .startof.(.dtb). If only Clang also supported that ...
>
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -162,7 +162,7 @@ past_zImage:
>
> /* Using the DTB in the .dtb section? */
> .ifnes CONFIG_DTB_FILE,""
> -load_paddr r8, _stdb
> +load_paddr r8, _sdtb
> .endif
>
> /* Initialize the UART if earlyprintk has been enabled. */
[PATCH] Arm32: restore proper name of .dtb section start symbol
This addresses a build failure when CONFIG_DTB_FILE evaluates to a non-
empty string.
Fixes: d07358f2dccd ("xen/arm32: head.S: Introduce a macro to load the physical
address of a symbol")
Signed-off-by: Jan Beulich
---
Of course this really would be a prime candidate for avoiding the
use of linker-script-defined symbols in the first place, by using
.startof.(.dtb). If only Clang also supported that ...
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -162,7 +162,7 @@ past_zImage:
/* Using the DTB in the .dtb section? */
.ifnes CONFIG_DTB_FILE,""
-load_paddr r8, _stdb
+load_paddr r8, _sdtb
.endif
/* Initialize the UART if earlyprintk has been enabled. */
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 18/07/2022 14:59, Boris Ostrovsky wrote:
>
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>> xen_hvm_smp_init();
>>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>> xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c
>>> b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>> xen_hvm_init_shared_info();
>>> xen_vcpu_restore();
>>> }
>>> - xen_setup_callback_vector();
>>> + if (xen_ack_upcall) {
>>> + unsigned int cpu;
>>> +
>>> + for_each_online_cpu(cpu) {
>>> + xen_hvm_evtchn_upcall_vector_t op = {
>>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> + };
>>> +
>>> +
>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> + ));
>>> + /* Trick toolstack to think we are enlightened. */
>>> + if (!cpu)
>>> + BUG_ON(xen_set_callback_via(1));
>> What are you trying to make the toolstack aware of? That we have *a*
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM
has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
>
>
> The hypercall has been around for a while so I understand ABI concerns
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
> Why not tie presence of this bit to no longer having to explicitly set
> the callback field?
Because that's a breaking change for ~5 years of windows drivers.
~Andrew
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 18/07/2022 14:59, Boris Ostrovsky wrote:
>
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>> xen_hvm_smp_init();
>>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>> xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>> xen_hvm_init_shared_info();
>>> xen_vcpu_restore();
>>> }
>>> - xen_setup_callback_vector();
>>> + if (xen_ack_upcall) {
>>> + unsigned int cpu;
>>> +
>>> + for_each_online_cpu(cpu) {
>>> + xen_hvm_evtchn_upcall_vector_t op = {
>>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> + };
>>> +
>>> +
>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> + ));
>>> + /* Trick toolstack to think we are enlightened. */
>>> + if (!cpu)
>>> + BUG_ON(xen_set_callback_via(1));
>> What are you trying to make the toolstack aware of? That we have *a*
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
>
>
> The hypercall has been around for a while so I understand ABI concerns
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
> Why not tie presence of this bit to no longer having to explicitly set
> the callback field?
>
Any other opinions on this?
(i.e., calling xen_set_callback_via(1) after
HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
recently added)
Thank you,
Jane.
[xen-unstable test] 171848: tolerable FAIL
flight 171848 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/171848/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail pass in 171823 test-amd64-i386-libvirt 7 xen-installfail pass in 171823 Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt 15 migrate-support-check fail in 171823 never pass test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 171823 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 171823 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 171823 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 171823 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 171823 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 171823 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 171823 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 171823 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 171823 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 171823 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 171823 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 171823 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass
Re: [PATCH] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently
On 25.07.2022 10:22, Xenia Ragiadakou wrote: > > On 7/25/22 11:00, Jan Beulich wrote: >> On 24.07.2022 19:31, Xenia Ragiadakou wrote: >>> The function snprintf() returns the number of characters that would have >>> been >>> written in the buffer if the buffer size had been sufficiently large, >>> not counting the terminating null character. >>> Hence, the value returned is not guaranteed to be smaller than the buffer >>> size. >>> Check the return value of snprintf to prevent leaking stack contents to the >>> guest by accident. >>> >>> Signed-off-by: Xenia Ragiadakou >>> --- >>> I 've noticed that in general in xen the return value of snprintf is not >>> checked. Is there a particular reason for this? I mean if there is no space >>> to >>> fit the entire string, is it preferable to write only a part of it instead >>> of >>> failing? If that's the case, then scnprintf could be used instead below. >> >> You will find lack of checking particularly in cases where the buffer size >> has been chosen to always fit the (expected) to-be-formatted value(s). >> While in a number of (most?) cases this ends up being fragile when >> considering general portability (like assuming that "unsigned int" can >> always be expressed in 10 decimal digits), I guess making such assumptions >> has been deemed "good enough" up until now. I think this also applies here, >> so ... >> >>> --- a/xen/common/hypfs.c >>> +++ b/xen/common/hypfs.c >>> @@ -377,6 +377,8 @@ int hypfs_read_dyndir_id_entry(const struct >>> hypfs_entry_dir *template, >>> unsigned int e_namelen, e_len; >>> >>> e_namelen = snprintf(name, sizeof(name), template->e.name, id); >>> +if ( e_namelen >= sizeof(name) ) >>> +return -ENOBUFS; >> >> ... I wonder whether you don't want to additionally put ASSERT_UNREACHABLE() >> here (but leave -ENOBUFS to keep release builds safe). (I also take it that >> you haven't found an actual case of the buffer being too small here?) > > hypfs_read_dyndir_id_entry() currently is used only by the cpupool > pooldir and the name needs to hold an unsigned int. So, currently there > is not a case of the buffer being too small. > >> >> But of course the main purpose of using snprintf() is to avoid buffer >> overrun, so truncation is indeed deemed only secondary in many cases. >> Which doesn't mean adding such checks would be unwelcome - it's just that >> in some of the cases your options are limited - see e.g. the other similar >> use of snprintf() in hypfs_gen_dyndir_id_entry(), where the function doesn't >> presently have any error cases. > > What I considered an issue here is that when copying the buffer to the > guest we use the value returned by snprintf(). > > copy_to_guest_offset(*uaddr, DIRENTRY_NAME_OFF, name, e_namelen + 1) > > Also, if truncation is not considered an issue, I can remove the check > and replace snprintf with scnprintf. Well, it is my view that truncation (if any could happen here) is an issue which does not want papering over. So I agree with you sticking to snprintf(). Still I think there's an internal assumption that truncation should not happen here, hence I've suggested the addition of an assertion to this effect. Jan
Re: [PATCH] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently
On 7/25/22 11:00, Jan Beulich wrote: On 24.07.2022 19:31, Xenia Ragiadakou wrote: The function snprintf() returns the number of characters that would have been written in the buffer if the buffer size had been sufficiently large, not counting the terminating null character. Hence, the value returned is not guaranteed to be smaller than the buffer size. Check the return value of snprintf to prevent leaking stack contents to the guest by accident. Signed-off-by: Xenia Ragiadakou --- I 've noticed that in general in xen the return value of snprintf is not checked. Is there a particular reason for this? I mean if there is no space to fit the entire string, is it preferable to write only a part of it instead of failing? If that's the case, then scnprintf could be used instead below. You will find lack of checking particularly in cases where the buffer size has been chosen to always fit the (expected) to-be-formatted value(s). While in a number of (most?) cases this ends up being fragile when considering general portability (like assuming that "unsigned int" can always be expressed in 10 decimal digits), I guess making such assumptions has been deemed "good enough" up until now. I think this also applies here, so ... --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -377,6 +377,8 @@ int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, unsigned int e_namelen, e_len; e_namelen = snprintf(name, sizeof(name), template->e.name, id); +if ( e_namelen >= sizeof(name) ) +return -ENOBUFS; ... I wonder whether you don't want to additionally put ASSERT_UNREACHABLE() here (but leave -ENOBUFS to keep release builds safe). (I also take it that you haven't found an actual case of the buffer being too small here?) hypfs_read_dyndir_id_entry() currently is used only by the cpupool pooldir and the name needs to hold an unsigned int. So, currently there is not a case of the buffer being too small. But of course the main purpose of using snprintf() is to avoid buffer overrun, so truncation is indeed deemed only secondary in many cases. Which doesn't mean adding such checks would be unwelcome - it's just that in some of the cases your options are limited - see e.g. the other similar use of snprintf() in hypfs_gen_dyndir_id_entry(), where the function doesn't presently have any error cases. What I considered an issue here is that when copying the buffer to the guest we use the value returned by snprintf(). copy_to_guest_offset(*uaddr, DIRENTRY_NAME_OFF, name, e_namelen + 1) Also, if truncation is not considered an issue, I can remove the check and replace snprintf with scnprintf. -- Xenia
Re: [PATCH v2] livepatch: create-diff-object: Check that the section has a secsym
On 25.07.2022 05:20, Sarah Newman wrote: > A STT_SECTION symbol is not needed if if it is not used as a relocation > target. Therefore, a section, in this case a debug section, may not have > a secsym associated with it. > > Origin: https://github.com/dynup/kpatch.git ba3defa06073 > Signed-off-by: Sarah Newman > --- > Changes in v2: > - commit message changed to use Origin With this I don't see why you didn't keep Ross'es R-b. Please help committers by keeping tags up-to-date. Jan
Re: [PATCH] xen/hypfs: check the return value of snprintf to avoid leaking stack accidently
On 24.07.2022 19:31, Xenia Ragiadakou wrote: > The function snprintf() returns the number of characters that would have been > written in the buffer if the buffer size had been sufficiently large, > not counting the terminating null character. > Hence, the value returned is not guaranteed to be smaller than the buffer > size. > Check the return value of snprintf to prevent leaking stack contents to the > guest by accident. > > Signed-off-by: Xenia Ragiadakou > --- > I 've noticed that in general in xen the return value of snprintf is not > checked. Is there a particular reason for this? I mean if there is no space to > fit the entire string, is it preferable to write only a part of it instead of > failing? If that's the case, then scnprintf could be used instead below. You will find lack of checking particularly in cases where the buffer size has been chosen to always fit the (expected) to-be-formatted value(s). While in a number of (most?) cases this ends up being fragile when considering general portability (like assuming that "unsigned int" can always be expressed in 10 decimal digits), I guess making such assumptions has been deemed "good enough" up until now. I think this also applies here, so ... > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -377,6 +377,8 @@ int hypfs_read_dyndir_id_entry(const struct > hypfs_entry_dir *template, > unsigned int e_namelen, e_len; > > e_namelen = snprintf(name, sizeof(name), template->e.name, id); > +if ( e_namelen >= sizeof(name) ) > +return -ENOBUFS; ... I wonder whether you don't want to additionally put ASSERT_UNREACHABLE() here (but leave -ENOBUFS to keep release builds safe). (I also take it that you haven't found an actual case of the buffer being too small here?) But of course the main purpose of using snprintf() is to avoid buffer overrun, so truncation is indeed deemed only secondary in many cases. Which doesn't mean adding such checks would be unwelcome - it's just that in some of the cases your options are limited - see e.g. the other similar use of snprintf() in hypfs_gen_dyndir_id_entry(), where the function doesn't presently have any error cases. Jan
Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
On 7/25/22 09:32, Jan Beulich wrote:
On 24.07.2022 19:20, Xenia Ragiadakou wrote:
On 7/7/22 10:55, Jan Beulich wrote:
On 07.07.2022 09:27, Xenia Ragiadakou wrote:
On 7/6/22 11:51, Jan Beulich wrote:
On 06.07.2022 10:43, Xenia Ragiadakou wrote:
On 7/6/22 10:10, Jan Beulich wrote:
On 05.07.2022 23:02, Xenia Ragiadakou wrote:
The function idle_loop() is referenced only in domain.c.
Change its linkage from external to internal by adding the storage-class
specifier static to its definitions.
Since idle_loop() is referenced only in inline assembly, add the 'used'
attribute to suppress unused-function compiler warning.
While I see that Julien has already acked the patch, I'd like to point
out that using __used here is somewhat bogus. Imo the better approach
is to properly make visible to the compiler that the symbol is used by
the asm(), by adding a fake(?) input.
I 'm afraid I do not understand what do you mean by "adding a fake(?)
input". Can you please elaborate a little on your suggestion?
Once the asm() in question takes the function as an input, the compiler
will know that the function has a user (unless, of course, it finds a
way to elide the asm() itself). The "fake(?)" was because I'm not deeply
enough into Arm inline assembly to know whether the input could then
also be used as an instruction operand (which imo would be preferable) -
if it can't (e.g. because there's no suitable constraint or operand
modifier), it still can be an input just to inform the compiler.
According to the following statement, your approach is the recommended one:
"To prevent the compiler from removing global data or functions which
are referenced from inline assembly statements, you can:
-use __attribute__((used)) with the global data or functions.
-pass the reference to global data or functions as operands to inline
assembly statements.
Arm recommends passing the reference to global data or functions as
operands to inline assembly statements so that if the final image does
not require the inline assembly statements and the referenced global
data or function, then they can be removed."
IIUC, you are suggesting to change
asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
into
asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
Yes, except that I can't judge about the "S" constraint.
This constraint does not work for arm32. Any other thoughts?
Another way, as Jan suggested, is to pass the function as a 'fake'
(unused) input.
I 'm suspecting something like the following could work
asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory")
What do you think?
Well, yes, X should always be a fallback option. But I said already when
you suggested S that I can't judge about its use, so I guess I'm the
wrong one to ask. Even more so that you only say "does not work", without
any details ...
The question is addressed to anyone familiar with arm inline assembly.
I added the arm maintainers as primary recipients to this email to make
this perfectly clear.
When cross-compiling Xen on x86 for arm32 with
asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
compilation fails with the error: impossible constraint in ‘asm'
--
Xenia
Re: [PATCH v1 05/18] x86: refactor xen cmdline into general framework
On 22.07.2022 15:12, Daniel P. Smith wrote:
> On 7/19/22 09:26, Jan Beulich wrote:
>> On 06.07.2022 23:04, Daniel P. Smith wrote:
>>> --- a/xen/include/xen/bootinfo.h
>>> +++ b/xen/include/xen/bootinfo.h
>>> @@ -53,6 +53,17 @@ struct __packed boot_info {
>>>
>>> extern struct boot_info *boot_info;
>>>
>>> +static inline char *bootinfo_prepare_cmdline(struct boot_info *bi)
>>> +{
>>> +bi->cmdline = arch_bootinfo_prepare_cmdline(bi->cmdline, bi->arch);
>>> +
>>> +if ( *bi->cmdline == ' ' )
>>> +printk(XENLOG_WARNING "%s: leading whitespace left on cmdline\n",
>>> + __func__);
>>
>> Just a remark and a question on this one: I don't view the use of
>> __func__ here (and in fact in many other cases as well) as very
>> useful. And why do we need such a warning all of the sudden in the
>> first place?
>
> This started as just a debug print, thus why __func__ is in place, but
> later decided to leave it. This is because after this point, the code
> assumes that all leading space was stripped, but there was never a check
> that logic did the job correct. I don't believe a failure to do so
> warranted a halt to the boot process, but at least provide a warning
> into the log should the trimming fail. Doing so would allow an admin to
> have a clue should an unexpected behavior occur as a result of leading
> space making it through and breaking the fully trimmed assumption made
> elsewhere.
All fine, but then such a change wants doing on its own, not in the middle
of pretty involved refactoring work.
Jan
Re: [PATCH v1 02/18] introduction of generalized boot info
On 22.07.2022 18:01, Daniel P. Smith wrote:
> On 7/21/22 12:00, Jan Beulich wrote:
>> On 21.07.2022 16:28, Daniel P. Smith wrote:
>>> On 7/19/22 09:11, Jan Beulich wrote:
On 06.07.2022 23:04, Daniel P. Smith wrote:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -0,0 +1,48 @@
> +#ifndef __ARCH_X86_BOOTINFO_H__
> +#define __ARCH_X86_BOOTINFO_H__
> +
> +/* unused for x86 */
> +struct arch_bootstring { };
> +
> +struct __packed arch_bootmodule {
> +#define BOOTMOD_FLAG_X86_RELOCATED 1U << 0
Such macro expansions need parenthesizing.
>>>
>>> Ack.
>>>
> +uint32_t flags;
> +uint32_t headroom;
> +};
Since you're not following any external spec, on top of what Julien
said about the __packed attribute I'd also like to point out that
in many cases here there's no need to use fixed-width types.
>>>
>>> Oh, I forgot to mention that in the reply to Julien. Yes, the __packed
>>> is needed to correctly cross the 32bit to 64bit bridge from the x86
>>> bootstrap in patch 4.
>>
>> I'm afraid I don't follow you here. I did briefly look at patch 4 (but
>> that really also falls in the "wants to be split" category), but I
>> can't see why a purely internally used struct may need packing. I'd
>> appreciate if you could expand on that.
>
> Originally, patch 3 and patch 4 were a single patch, and obviously was
> way too large. To split them, I realized I could introduce a temporary
> conversion function that would allow the patch to be split into a post
> start_xen() patch (patch 3) and a pre start_xen() patch, (patch 4). For
> x86, pre start_xen() consists of 3 different entry points. These being
> the classic/traditional/old multiboot1/2 entry, EFI entry, and PVH entry
> (aka Xen Guest). The latter two are all internal, 64bit, but the former
> is located in arch/x86/boot and is compiled as 32bit. I tried different
> approaches to support using a single header between these two
> environments. Ultimately, IMHO, the cleanest approach is what is
> introduced in patch 4 as it enabled the use of Xen types in the
> structures and maintain a single structure that need to be passed
> around. To do this, a 32bit specific version of the structures were
> defined in arch/x86/boot/boot_info32.h that is populated under 32bit
> mode, then they can be fixed up after getting into start_xen() and in
> 64bit code. To ensure no unexpected insertion of padding, I focused on
> ensuring everything was 32bit aligned and packed. As Julien pointed out,
> I messed up with the use of enum as its size is not guaranteed as the
> enum list grows and I forgot to consider keeping pointers 64bit aligned.
>
> Does that help?
It helps as background info, yes, but I continue to be unhappy with the
new uses of the __packed attribute.
> +struct __packed arch_boot_info {
> +uint32_t flags;
> +#define BOOTINFO_FLAG_X86_MEMLIMITS 1U << 0
> +#define BOOTINFO_FLAG_X86_BOOTDEV1U << 1
> +#define BOOTINFO_FLAG_X86_CMDLINE1U << 2
> +#define BOOTINFO_FLAG_X86_MODULES1U << 3
> +#define BOOTINFO_FLAG_X86_AOUT_SYMS 1U << 4
> +#define BOOTINFO_FLAG_X86_ELF_SYMS 1U << 5
> +#define BOOTINFO_FLAG_X86_MEMMAP 1U << 6
> +#define BOOTINFO_FLAG_X86_DRIVES 1U << 7
> +#define BOOTINFO_FLAG_X86_BIOSCONFIG 1U << 8
> +#define BOOTINFO_FLAG_X86_LOADERNAME 1U << 9
> +#define BOOTINFO_FLAG_X86_APM1U << 10
> +
> +bool xen_guest;
As the example of this, with just the header files being introduced
here it is not really possible to figure what these fields are to
be used for and hence whether they're legitimately represented here.
>>>
>>> I can add a comment to clarify these are a mirror of the multiboot
>>> flags. These were mirrored to allow the multiboot flags to be direct
>>> copied and eased the replacement locations where an mb flag is checked.
>>
>> Multiboot flags? The context here is the "xen_guest" field.
>
> Apologies, I thought you were referring to all the fields and I forgot
> to explain xen_guest. So to clarify, flags is to carry the MB flags
> passed up from the MB entry point and xen_guest is meant to carry the
> xen_guest bool passed up from the PVH/Xen Guest entry point.
That was my guess, but then my request stands: The fields should be
added to the struct at the time they're being made use of.
Jan
[libvirt test] 171850: regressions - FAIL
flight 171850 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/171850/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt 510540961417288a24d0870f0226f8255420c463 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 745 days Failing since151818 2020-07-11 04:18:52 Z 744 days 726 attempts Testing same since 171795 2022-07-23 04:21:17 Z2 days3 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Amneesh Singh Andika Triwidada Andrea Bolognani Andrew Melnychenko Ani Sinha Balázs Meskó Barrett Schonefeld Bastian Germann Bastien Orivel BiaoXiang Ye Bihong Yu Binfeng Wu Bjoern Walk Boris Fiuczynski Brad Laue Brian Turek Bruno Haible Chris Mayo Christian Borntraeger Christian Ehrhardt Christian Kirbach Christian Schoenebeck Christophe Fergeau Claudio Fontana Cole Robinson Collin Walling Cornelia Huck Cédric Bosdonnat Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé David Michael Didik Supriadi dinglimin Divya Garg Dmitrii Shcherbakov Dmytro Linkin Eiichi Tsukata Emilio Herrera Eric Farman Erik Skultety Eugenio Pérez Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation Florian Schmidt Franck Ridel Gavi Teitz gongwei Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Haonan Wang Hela Basa Helmut Grohne Hiroki Narukawa Hyman Huang(黄勇) Ian Wienand Ioanna Alifieraki Ivan Teterevkov Jakob Meng Jamie Strandboge Jamie Strandboge Jan Kuparinen jason lee Jean-Baptiste Holcroft Jia Zhou Jianan Gao Jim Fehlig Jin Yan Jing Qi Jinsheng Zhang Jiri Denemark Joachim Falk John Ferlan John Levon John Levon Jonathan Watt Jonathon Jongsma Julio Faracco Justin Gatzen Ján Tomko Kashyap Chamarthy Kevin Locke Kim InSoo Koichi Murase Kristina Hanicova Laine Stump Laszlo Ersek Lee Yarwood Lei Yang Lena Voytek Liang Yan Liang Yan Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Lubomir Rintel Luke Yue Luyao Zhong luzhipeng Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Mark Mielke Markus Schade Martin Kletzander Martin Pitt Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Max Goodhart Maxim Nestratov Meina Li Michal Privoznik Michał Smyk Milo Casagrande minglei.liu Moshe Levi Moteen Shah Moteen Shah Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nicolas Lécureuil Nicolas Lécureuil Nikolay Shirokovskiy Nikolay Shirokovskiy Nikolay Shirokovskiy Niteesh Dubey Olaf Hering Olesya Gerasimenko Or Ozeri Orion Poplawski Pany Paolo Bonzini Patrick Magauran
Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
On 24.07.2022 19:20, Xenia Ragiadakou wrote:
> On 7/7/22 10:55, Jan Beulich wrote:
>> On 07.07.2022 09:27, Xenia Ragiadakou wrote:
>>> On 7/6/22 11:51, Jan Beulich wrote:
On 06.07.2022 10:43, Xenia Ragiadakou wrote:
> On 7/6/22 10:10, Jan Beulich wrote:
>> On 05.07.2022 23:02, Xenia Ragiadakou wrote:
>>> The function idle_loop() is referenced only in domain.c.
>>> Change its linkage from external to internal by adding the storage-class
>>> specifier static to its definitions.
>>>
>>> Since idle_loop() is referenced only in inline assembly, add the 'used'
>>> attribute to suppress unused-function compiler warning.
>>
>> While I see that Julien has already acked the patch, I'd like to point
>> out that using __used here is somewhat bogus. Imo the better approach
>> is to properly make visible to the compiler that the symbol is used by
>> the asm(), by adding a fake(?) input.
>
> I 'm afraid I do not understand what do you mean by "adding a fake(?)
> input". Can you please elaborate a little on your suggestion?
Once the asm() in question takes the function as an input, the compiler
will know that the function has a user (unless, of course, it finds a
way to elide the asm() itself). The "fake(?)" was because I'm not deeply
enough into Arm inline assembly to know whether the input could then
also be used as an instruction operand (which imo would be preferable) -
if it can't (e.g. because there's no suitable constraint or operand
modifier), it still can be an input just to inform the compiler.
>>>
>>> According to the following statement, your approach is the recommended one:
>>> "To prevent the compiler from removing global data or functions which
>>> are referenced from inline assembly statements, you can:
>>> -use __attribute__((used)) with the global data or functions.
>>> -pass the reference to global data or functions as operands to inline
>>> assembly statements.
>>> Arm recommends passing the reference to global data or functions as
>>> operands to inline assembly statements so that if the final image does
>>> not require the inline assembly statements and the referenced global
>>> data or function, then they can be removed."
>>>
>>> IIUC, you are suggesting to change
>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
>>> into
>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" );
>>
>> Yes, except that I can't judge about the "S" constraint.
>>
>
> This constraint does not work for arm32. Any other thoughts?
>
> Another way, as Jan suggested, is to pass the function as a 'fake'
> (unused) input.
> I 'm suspecting something like the following could work
> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory")
> What do you think?
Well, yes, X should always be a fallback option. But I said already when
you suggested S that I can't judge about its use, so I guess I'm the
wrong one to ask. Even more so that you only say "does not work", without
any details ...
Jan
