Re: [PATCH v2 37/39] xen/rirscv: add minimal amount of stubs to build full Xen
On 20.12.2023 13:55, Oleksii wrote: > On Mon, 2023-12-18 at 18:00 +0100, Jan Beulich wrote: >> On 24.11.2023 11:30, Oleksii Kurochko wrote: >>> --- /dev/null >>> +++ b/xen/arch/riscv/stubs.c >>> @@ -0,0 +1,426 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> I think I can see why you need the former of these last two, but do >> you >> really need the latter? > It is needed for vm_event_request_t and vm_event_response_t, but if use > a forward declaration that it won't be needed: > > typedef struct vm_event_st vm_event_request_t; > typedef struct vm_event_st vm_event_response_t; Iirc Misra wouldn't like the duplicating of typedef-s used elsewhere. But as long as that's not going to stay (and I expect stubs.c to go away before Misra becomes of concern for RISC-V), that's going to be okay, I think. Yet then avoiding the typedef-s and using struct vm_event_st directly in the functions would be as good, and overall less code. Jan
Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
On 20.12.2023 13:15, Federico Serafini wrote: > On 20/12/23 12:55, Jan Beulich wrote: >> On 20.12.2023 12:48, Julien Grall wrote: >>> On 20/12/2023 11:03, Federico Serafini wrote: --- a/xen/arch/arm/arm64/vsysreg.c +++ b/xen/arch/arm/arm64/vsysreg.c @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, /* RO at EL0. RAZ/WI at EL1 */ if ( regs_mode_is_user(regs) ) return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); -else -return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); + +return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>> >>> I don't 100% like this change (mostly because I find if/else clearer >>> here). >> >> While (it doesn't matter here) my view on this is different, I'm still >> puzzled why the tool would complain / why a change here is necessary. >> It is not _one_ return statement, but there's still (and obviously) no >> way of falling through. > > The tool is configurable: > if you prefer deviate these cases instead of refactoring the code > I can update the configuration. I guess this then needs to be discussed on the first call in the new year. Stefano - can you take note of that, please? Jan
Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands
On 2023/12/19 16:53, Huang Rui wrote: From: Antonio Caggiano Support BLOB resources creation, mapping and unmapping by calling the new stable virglrenderer 0.10 interface. Only enabled when available and via the blob config. E.g. -device virtio-vga-gl,blob=true I have another concern about delaying virgl_renderer_resource_unref() until the resource gets unmapped; the guest will expect the resource ID will be available for a new resource immediately after VIRTIO_GPU_CMD_RESOURCE_UNREF, but it will break the assumption and may corrupt things.
Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3
On 20.12.2023 22:35, Andrew Cooper wrote:
> On 20/12/2023 11:03 am, Federico Serafini wrote:
>> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
>> code. No fucntional changes are introduced.
>>
>> Federico Serafini (7):
>> xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>> xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>> xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>> xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>> xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>> xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>> xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>>
>> xen/arch/arm/arm64/vsysreg.c | 4 ++--
>> xen/arch/arm/gic-v3.c | 30 +++
>> xen/arch/arm/guest_walk.c | 4
>> xen/arch/arm/mem_access.c | 12 +--
>> xen/arch/arm/mmu/p2m.c| 1 +
>> xen/arch/arm/traps.c | 18
>> xen/arch/arm/vcpreg.c | 4 ++--
>> xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
>> 8 files changed, 61 insertions(+), 14 deletions(-)
>>
>
> Just a couple of notes on style. This isn't a request to change
> anything in this series, particularly as most is already committed, but
> bear it in mind for what I expect will be similar patches in other areas.
>
> We explicitly permit tabulation when it aids readibility, so patch 2
> could have been written:
>
> switch ( hypercall_args[*nr] ) {
> case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
> case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
> case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
> case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
> case 1: /* Don't clobber x0/r0 -- it's the return value */
> case 0: /* -ENOSYS case */
> break;
> default: BUG();
> }
>
> (give or take the brace placement other style issue) We also have cases
> where a break before a new case statement is preferred, i.e.:
Did you mean "blank line" here, seeing ...
> ...
> break;
>
> case ...:
>
> This is to prevent larger switch statements from being a straight wall
> of text.
... this as the further explanation?
Jan
Re: [PATCH v4 2/6] amd-vi: set IOMMU page table levels based on guest reported paddr width
On 20.12.2023 14:43, Roger Pau Monne wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -359,21 +359,17 @@ int __read_mostly amd_iommu_min_paging_mode = 1;
> static int cf_check amd_iommu_domain_init(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
> +int pglvl = amd_iommu_get_paging_mode(
> +1UL << (domain_max_paddr_bits(d) - PAGE_SHIFT));
Nit: Considering the "pending" open parenthesis, I think this line
needs indenting one level further. I'll make the adjustment while
committing, unless I hear objections pretty soon.
Jan
Re: [PATCH v11.5 1/17] pci: msi: pass pdev to pci_enable_msi() function
On 20.12.2023 22:46, Stewart Hildebrand wrote: > From: Volodymyr Babchuk > > Previously pci_enable_msi() function obtained pdev pointer by itself, > but taking into account upcoming changes to PCI locking, it is better > when caller passes already acquired pdev pointer to the function, > because caller knows better how to obtain the pointer and which locks > are needed to be used. Also, in most cases caller already has pointer > to pdev, so we can avoid an extra list walk. > > Signed-off-by: Volodymyr Babchuk > Signed-off-by: Stewart Hildebrand > Reviewed-by: Stefano Stabellini Reviewed-by: Jan Beulich
Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions
On 21/12/23 09:50, Akihiko Odaki wrote: On 2023/12/21 16:35, Xenia Ragiadakou wrote: On 21/12/23 07:45, Akihiko Odaki wrote: On 2023/12/19 16:53, Huang Rui wrote: From: Xenia Ragiadakou When the memory region has a different life-cycle from that of her parent, could be automatically released, once has been unparent and once all of her references have gone away, via the object's free callback. However, currently, the address space subsystem keeps references to the memory region without first incrementing its object's reference count. As a result, the automatic deallocation of the object, not taking into account those references, results in use-after-free memory corruption. More specifically, reference to the memory region is kept in flatview ranges. If the reference count of the memory region is not incremented, flatview_destroy(), that is asynchronous, may be called after memory region's destruction. If the reference count of the memory region is incremented, memory region's destruction will take place after flatview_destroy() has released its references. This patch increases the reference count of an owned memory region object on each memory_region_ref() and decreases it on each memory_region_unref(). Why not pass the memory region itself as the owner parameter of memory_region_init_ram_ptr()? Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't disappear while the memory region is still in use? You can object_ref() when you do memory_region_init_ram_ptr() and object_unref() when the memory region is being destroyed. It is not very intuitive but I see your point. This change is quite intrusive and has little use. I think it can be worked around in the way you suggest.
[linux-linus test] 184194: tolerable FAIL - PUSHED
flight 184194 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/184194/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184179 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184179 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184179 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184179 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184179 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184179 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184179 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184179 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-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-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-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 test-armhf-armhf-xl-rtds 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-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-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: linux1a44b0073b9235521280e19d963b6dfef7888f18 baseline version: linux55cb5f43689d7a9ea5bf35ef050f12334f197347 Last test of basis 184179 2023-12-19 21:12:29 Z1 days Testing same since 184194 2023-12-20 20:40:45 Z0 days1 attempts People who touched revisions under test: Amir Goldstein Chuck Lever Daniel Hill Jan Kara Kent Overstreet Linus Torvalds Mike Snitzer Mikulas Patocka NeilBrown Thomas Bertschinger Xiao Ni Yu Kuai jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-li
Re: [PATCH] x86/amd: extend CPU errata #1474 affected models
On 20.12.2023 16:42, Andrew Cooper wrote: > On 20/12/2023 3:10 pm, Roger Pau Monné wrote: >> On Wed, Dec 20, 2023 at 02:46:43PM +, Andrew Cooper wrote: >>> On 20/12/2023 2:22 pm, Roger Pau Monne wrote: @@ -978,24 +978,24 @@ void amd_check_zenbleed(void) val & chickenbit ? "chickenbit" : "microcode"); } -static void cf_check zen2_disable_c6(void *arg) +static void cf_check zen_disable_c6(void *arg) >>> fam17_disable_c6() ? I know Hygon is 0x18 but it's also reasonably well >>> know to be the same uarch. >>> >>> This particular algorithm is good for all Fam17 uarches, irrespective of >>> #1474, even if they happen to be the same set of CPUs in practice. >> Yeah, I was about to use fam17h prefix, but that wouldn't cover Hygon. >> I we are fine with it I can send an adjusted v2 using fam17h prefix. > > I think we're fine calling it fam17. Happy to do that consistently. I agree. And it clearly cannot be just "zen", to avoid it also wrongly covering Zen3 / Zen4. Jan
[PATCH RFC v3 for-6.8/block 02/17] xen/blkback: use bdev api in xen_update_blkif_status()
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_devcie.
Signed-off-by: Yu Kuai
---
drivers/block/xen-blkback/xenbus.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c
b/drivers/block/xen-blkback/xenbus.c
index e34219ea2b05..e645afa4af57 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -104,8 +104,7 @@ static void xen_update_blkif_status(struct xen_blkif *blkif)
xenbus_dev_error(blkif->be->dev, err, "block flush");
return;
}
- invalidate_inode_pages2(
- blkif->vbd.bdev_handle->bdev->bd_inode->i_mapping);
+ invalidate_bdev(blkif->vbd.bdev_handle->bdev);
for (i = 0; i < blkif->nr_rings; i++) {
ring = &blkif->rings[i];
--
2.39.2
[PATCH RFC v3 for-6.8/block 04/17] mtd: block2mtd: use bdev apis
From: Yu Kuai
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai
---
drivers/mtd/devices/block2mtd.c | 81 +++--
1 file changed, 36 insertions(+), 45 deletions(-)
diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index aa44a23ec045..cf201bf73184 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -46,40 +46,34 @@ struct block2mtd_dev {
/* Static info about the MTD, used in cleanup_module */
static LIST_HEAD(blkmtd_device_list);
-
-static struct page *page_read(struct address_space *mapping, pgoff_t index)
-{
- return read_mapping_page(mapping, index, NULL);
-}
-
/* erase a specified part of the device */
static int _block2mtd_erase(struct block2mtd_dev *dev, loff_t to, size_t len)
{
- struct address_space *mapping =
- dev->bdev_handle->bdev->bd_inode->i_mapping;
- struct page *page;
+ struct block_device *bdev = dev->bdev_handle->bdev;
+ struct folio *folio;
pgoff_t index = to >> PAGE_SHIFT; // page index
int pages = len >> PAGE_SHIFT;
u_long *p;
u_long *max;
while (pages) {
- page = page_read(mapping, index);
- if (IS_ERR(page))
- return PTR_ERR(page);
+ folio = bdev_read_folio(bdev, index << PAGE_SHIFT);
+ if (IS_ERR(folio))
+ return PTR_ERR(folio);
- max = page_address(page) + PAGE_SIZE;
- for (p=page_address(page); ppriv;
- struct address_space *mapping =
- dev->bdev_handle->bdev->bd_inode->i_mapping;
- struct page *page;
+ struct folio *folio;
pgoff_t index = from >> PAGE_SHIFT;
int offset = from & (PAGE_SIZE-1);
int cpylen;
@@ -120,12 +112,13 @@ static int block2mtd_read(struct mtd_info *mtd, loff_t
from, size_t len,
cpylen = len; // this page
len = len - cpylen;
- page = page_read(mapping, index);
- if (IS_ERR(page))
- return PTR_ERR(page);
+ folio = bdev_read_folio(dev->bdev_handle->bdev,
+ index << PAGE_SHIFT);
+ if (IS_ERR(folio))
+ return PTR_ERR(folio);
- memcpy(buf, page_address(page) + offset, cpylen);
- put_page(page);
+ memcpy(buf, folio_address(folio) + offset, cpylen);
+ folio_put(folio);
if (retlen)
*retlen += cpylen;
@@ -141,9 +134,8 @@ static int block2mtd_read(struct mtd_info *mtd, loff_t
from, size_t len,
static int _block2mtd_write(struct block2mtd_dev *dev, const u_char *buf,
loff_t to, size_t len, size_t *retlen)
{
- struct page *page;
- struct address_space *mapping =
- dev->bdev_handle->bdev->bd_inode->i_mapping;
+ struct block_device *bdev = dev->bdev_handle->bdev;
+ struct folio *folio;
pgoff_t index = to >> PAGE_SHIFT; // page index
int offset = to & ~PAGE_MASK; // page offset
int cpylen;
@@ -155,18 +147,18 @@ static int _block2mtd_write(struct block2mtd_dev *dev,
const u_char *buf,
cpylen = len; // this page
len = len - cpylen;
- page = page_read(mapping, index);
- if (IS_ERR(page))
- return PTR_ERR(page);
+ folio = bdev_read_folio(bdev, index << PAGE_SHIFT);
+ if (IS_ERR(folio))
+ return PTR_ERR(folio);
- if (memcmp(page_address(page)+offset, buf, cpylen)) {
- lock_page(page);
- memcpy(page_address(page) + offset, buf, cpylen);
- set_page_dirty(page);
- unlock_page(page);
- balance_dirty_pages_ratelimited(mapping);
+ if (memcmp(folio_address(folio) + offset, buf, cpylen)) {
+ folio_lock(folio);
+ memcpy(folio_address(folio) + offset, buf, cpylen);
+ folio_mark_dirty(folio);
+ folio_unlock(folio);
+ bdev_balance_dirty_pages_ratelimited(bdev);
}
- put_page(page);
+ folio_put(folio);
if (retlen)
*retlen += cpylen;
@@ -211,8 +203,7 @@ static void block2mtd_free_device(struct block2mtd_dev *dev)
kfree(dev->mtd.name);
if (dev->bdev_handle) {
- invalidate_mapping_pages(
- dev->bdev_handle->bdev->bd_inode->i_mapping, 0, -1);
+ invalidate_bdev(dev->bdev_h
[PATCH RFC v3 for-6.8/block 00/17] block: don't access bd_inode directly from other modules
From: Yu Kuai Changes in v3: - remove bdev_associated_mapping() and patch 12 from v1; - add kerneldoc comments for new bdev apis; - rename __bdev_get_folio() to bdev_get_folio; - fix a problem in erofs that erofs_init_metabuf() is not always called. - add reviewed-by tag for patch 15-17; Changes in v2: - remove some bdev apis that is not necessary; - pass in offset for bdev_read_folio() and __bdev_get_folio(); - remove bdev_gfp_constraint() and add a new helper in fs/buffer.c to prevent access bd_indoe() directly from mapping_gfp_constraint() in ext4.(patch 15, 16); - remove block_device_ejected() from ext4. Patch 1 add some bdev apis, then follow up patches will use these apis to avoid access bd_inode directly, and hopefully the field bd_inode can be removed eventually(after figure out a way for fs/buffer.c). Yu Kuai (17): block: add some bdev apis xen/blkback: use bdev api in xen_update_blkif_status() bcache: use bdev api in read_super() mtd: block2mtd: use bdev apis s390/dasd: use bdev api in dasd_format() scsicam: use bdev api in scsi_bios_ptable() bcachefs: remove dead function bdev_sectors() bio: export bio_add_folio_nofail() btrfs: use bdev apis cramfs: use bdev apis in cramfs_blkdev_read() erofs: use bdev api nilfs2: use bdev api in nilfs_attach_log_writer() jbd2: use bdev apis buffer: add a new helper to read sb block ext4: use new helper to read sb block ext4: remove block_device_ejected() ext4: use bdev apis block/bdev.c | 148 + block/bio.c| 1 + block/blk.h| 2 - drivers/block/xen-blkback/xenbus.c | 3 +- drivers/md/bcache/super.c | 11 +-- drivers/mtd/devices/block2mtd.c| 81 +++- drivers/s390/block/dasd_ioctl.c| 5 +- drivers/scsi/scsicam.c | 4 +- fs/bcachefs/util.h | 5 - fs/btrfs/disk-io.c | 71 +++--- fs/btrfs/volumes.c | 17 ++-- fs/btrfs/zoned.c | 15 +-- fs/buffer.c| 68 + fs/cramfs/inode.c | 36 +++ fs/erofs/data.c| 18 ++-- fs/erofs/internal.h| 2 + fs/ext4/dir.c | 6 +- fs/ext4/ext4.h | 13 --- fs/ext4/ext4_jbd2.c| 6 +- fs/ext4/inode.c| 8 +- fs/ext4/super.c| 66 +++-- fs/ext4/symlink.c | 2 +- fs/jbd2/journal.c | 3 +- fs/jbd2/recovery.c | 6 +- fs/nilfs2/segment.c| 2 +- include/linux/blkdev.h | 17 include/linux/buffer_head.h| 18 +++- 27 files changed, 377 insertions(+), 257 deletions(-) -- 2.39.2
[PATCH RFC v3 for-6.8/block 07/17] bcachefs: remove dead function bdev_sectors()
From: Yu Kuai
bdev_sectors() is not used hence remove it.
Signed-off-by: Yu Kuai
---
fs/bcachefs/util.h | 5 -
1 file changed, 5 deletions(-)
diff --git a/fs/bcachefs/util.h b/fs/bcachefs/util.h
index 2984b57b2958..22a0acc1704f 100644
--- a/fs/bcachefs/util.h
+++ b/fs/bcachefs/util.h
@@ -516,11 +516,6 @@ static inline unsigned fract_exp_two(unsigned x, unsigned
fract_bits)
void bch2_bio_map(struct bio *bio, void *base, size_t);
int bch2_bio_alloc_pages(struct bio *, size_t, gfp_t);
-static inline sector_t bdev_sectors(struct block_device *bdev)
-{
- return bdev->bd_inode->i_size >> 9;
-}
-
#define closure_bio_submit(bio, cl)\
do { \
closure_get(cl);\
--
2.39.2
[PATCH RFC v3 for-6.8/block 05/17] s390/dasd: use bdev api in dasd_format()
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_devcie.
Signed-off-by: Yu Kuai
---
drivers/s390/block/dasd_ioctl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index 61b9675e2a67..bbfb958237e6 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -221,8 +221,9 @@ dasd_format(struct dasd_block *block, struct format_data_t
*fdata)
* enabling the device later.
*/
if (fdata->start_unit == 0) {
- block->gdp->part0->bd_inode->i_blkbits =
- blksize_bits(fdata->blksize);
+ rc = set_blocksize(block->gdp->part0, fdata->blksize);
+ if (rc)
+ return rc;
}
rc = base->discipline->format_device(base, fdata, 1);
--
2.39.2
[PATCH RFC v3 for-6.8/block 10/17] cramfs: use bdev apis in cramfs_blkdev_read()
From: Yu Kuai
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Also do some cleanup that there is no need for two for loop, and remove
local array pages.
Signed-off-by: Yu Kuai
---
fs/cramfs/inode.c | 36 +---
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 60dbfa0f8805..fad95d683d97 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -183,9 +183,6 @@ static int next_buffer;
static void *cramfs_blkdev_read(struct super_block *sb, unsigned int offset,
unsigned int len)
{
- struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
- struct file_ra_state ra = {};
- struct page *pages[BLKS_PER_BUF];
unsigned i, blocknr, buffer;
unsigned long devsize;
char *data;
@@ -214,37 +211,30 @@ static void *cramfs_blkdev_read(struct super_block *sb,
unsigned int offset,
devsize = bdev_nr_bytes(sb->s_bdev) >> PAGE_SHIFT;
/* Ok, read in BLKS_PER_BUF pages completely first. */
- file_ra_state_init(&ra, mapping);
- page_cache_sync_readahead(mapping, &ra, NULL, blocknr, BLKS_PER_BUF);
-
- for (i = 0; i < BLKS_PER_BUF; i++) {
- struct page *page = NULL;
-
- if (blocknr + i < devsize) {
- page = read_mapping_page(mapping, blocknr + i, NULL);
- /* synchronous error? */
- if (IS_ERR(page))
- page = NULL;
- }
- pages[i] = page;
- }
+ bdev_sync_readahead(sb->s_bdev, NULL, NULL, blocknr, BLKS_PER_BUF);
buffer = next_buffer;
next_buffer = NEXT_BUFFER(buffer);
buffer_blocknr[buffer] = blocknr;
buffer_dev[buffer] = sb;
-
data = read_buffers[buffer];
+
for (i = 0; i < BLKS_PER_BUF; i++) {
- struct page *page = pages[i];
+ struct folio *folio = NULL;
+
+ if (blocknr + i < devsize)
+ folio = bdev_read_folio(sb->s_bdev,
+ (blocknr + i) << PAGE_SHIFT);
- if (page) {
- memcpy_from_page(data, page, 0, PAGE_SIZE);
- put_page(page);
- } else
+ if (IS_ERR_OR_NULL(folio)) {
memset(data, 0, PAGE_SIZE);
+ } else {
+ memcpy_from_folio(data, folio, 0, PAGE_SIZE);
+ folio_put(folio);
+ }
data += PAGE_SIZE;
}
+
return read_buffers[buffer] + offset;
}
--
2.39.2
[PATCH RFC v3 for-6.8/block 01/17] block: add some bdev apis
From: Yu Kuai
Those apis will be used for other modules, so that bd_inode won't be
accessed directly from other modules.
Signed-off-by: Yu Kuai
---
block/bdev.c | 148 +
block/blk.h| 2 -
include/linux/blkdev.h | 17 +
3 files changed, 165 insertions(+), 2 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index 750aec178b6a..6204621c6db6 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -89,6 +89,25 @@ void invalidate_bdev(struct block_device *bdev)
}
EXPORT_SYMBOL(invalidate_bdev);
+/**
+ * invalidate_bdev_pages - Invalidate clean unused buffers and pagecache.
+ * @bdev: the block device which holds the cache to invalidate
+ * @start: the offset 'from' which to invalidate
+ * @end: the offset 'to' which to invalidate (inclusive)
+ *
+ * This function removes pages that are clean, unmapped and unlocked,
+ * as well as shadow entries. It will not block on IO activity.
+ *
+ * If you want to remove all the pages of one block device, regardless of
+ * their use and writeback state, use truncate_bdev_range().
+ */
+void invalidate_bdev_range(struct block_device *bdev, pgoff_t start,
+ pgoff_t end)
+{
+ invalidate_mapping_pages(bdev->bd_inode->i_mapping, start, end);
+}
+EXPORT_SYMBOL_GPL(invalidate_bdev_range);
+
/*
* Drop all buffers & page cache for given bdev range. This function bails
* with error if bdev has other exclusive owner (such as filesystem).
@@ -121,6 +140,7 @@ int truncate_bdev_range(struct block_device *bdev,
blk_mode_t mode,
lstart >> PAGE_SHIFT,
lend >> PAGE_SHIFT);
}
+EXPORT_SYMBOL_GPL(truncate_bdev_range);
static void set_init_blocksize(struct block_device *bdev)
{
@@ -1102,3 +1122,131 @@ void bdev_statx_dioalign(struct inode *inode, struct
kstat *stat)
blkdev_put_no_open(bdev);
}
+
+/**
+ * bdev_read_folio - Read into block device page cache.
+ * @bdev: the block device which holds the cache to read.
+ * @pos: the offset that allocated folio will contain.
+ *
+ * Read one page into the block device page cache. If it succeeds, the folio
+ * returned will contain @pos;
+ *
+ * Return: Uptodate folio on success, ERR_PTR() on failure.
+ */
+struct folio *bdev_read_folio(struct block_device *bdev, loff_t pos)
+{
+ return mapping_read_folio_gfp(bdev->bd_inode->i_mapping,
+ pos >> PAGE_SHIFT, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(bdev_read_folio);
+
+/**
+ * bdev_get_folio - Find and get a reference to a folio.
+ * @bdev: the block device which holds the address_space to search.
+ * @pos: the offset the returned folio will contain.
+ * @fgp_flags: %FGP flags modify how the folio is returned.
+ * @gfp: Memory allocation flags to use if %FGP_CREAT is specified.
+ *
+ * Looks up the page cache entry at @bdev->bd_inode->i_mapping from @pos. If
+ * this function returns a folio, it is returned with an increased refcount.
+ *
+ * Return: The found folio or an ERR_PTR() otherwise.
+ */
+struct folio *bdev_get_folio(struct block_device *bdev, loff_t pos,
+fgf_t fgp_flags, gfp_t gfp)
+{
+ return __filemap_get_folio(bdev->bd_inode->i_mapping, pos >> PAGE_SHIFT,
+ fgp_flags, gfp);
+}
+EXPORT_SYMBOL_GPL(bdev_get_folio);
+
+/**
+ * bdev_wb_err_check - Has block device writeback error occurred?
+ * @bdev: the block device to check.
+ * @since: Previously-sampled @bdev->bd_inode->i_mapping->wb_err.
+ *
+ * Grab @bdev->bd_inode->i_mapping->wb_err, and see if it has changed @since
+ * the given value was sampled.
+ *
+ * Return: The latest error or 0 if it hasn't changed.
+ */
+int bdev_wb_err_check(struct block_device *bdev, errseq_t since)
+{
+ return errseq_check(&bdev->bd_inode->i_mapping->wb_err, since);
+}
+EXPORT_SYMBOL_GPL(bdev_wb_err_check);
+
+/**
+ * bdev_wb_err_check_and_advance() - Check block device writeback error and
+ * advance to current value.
+ * @bdev: the block device to check;
+ * @since: Pointer to previously-sampled @bdev->bd_inode->i_mapping->wb_err to
+ * check against and advance.
+ *
+ * Grab @bdev->bd_inode->i_mapping->wb_err, and see whether it matches the
+ * value that @since points to. If it does, then just return 0; If it doesn't,
+ * then the value has changed. Set the "seen" flag, and try to swap it into
+ * place as the new eseq value. Then, set that value as the new @since value,
+ * and return whatever the error portion is set to.
+ *
+ * Return: Negative errno if one has been stored, or 0 if no new error has
+ * occurred.
+ */
+int bdev_wb_err_check_and_advance(struct block_device *bdev, errseq_t *since)
+{
+ return errseq_check_and_advance(&bdev->bd_inode->i_mapping->wb_err,
+ since);
+}
+EXPORT_SYMBOL_GPL(bdev_wb_err_check_and_advance);
+
+/**
+ * bdev_balance_dirty_pages_ratelimited - balance di
[PATCH RFC v3 for-6.8/block 03/17] bcache: use bdev api in read_super()
From: Yu Kuai
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai
---
drivers/md/bcache/super.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index bfe1685dbae5..23892b32c582 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -168,14 +168,13 @@ static const char *read_super(struct cache_sb *sb, struct
block_device *bdev,
{
const char *err;
struct cache_sb_disk *s;
- struct page *page;
+ struct folio *folio;
unsigned int i;
- page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
- SB_OFFSET >> PAGE_SHIFT, GFP_KERNEL);
- if (IS_ERR(page))
+ folio = bdev_read_folio(bdev, SB_OFFSET);
+ if (IS_ERR(folio))
return "IO error";
- s = page_address(page) + offset_in_page(SB_OFFSET);
+ s = folio_address(folio) + offset_in_folio(folio, SB_OFFSET);
sb->offset = le64_to_cpu(s->offset);
sb->version = le64_to_cpu(s->version);
@@ -272,7 +271,7 @@ static const char *read_super(struct cache_sb *sb, struct
block_device *bdev,
*res = s;
return NULL;
err:
- put_page(page);
+ folio_put(folio);
return err;
}
--
2.39.2
[PATCH RFC v3 for-6.8/block 06/17] scsicam: use bdev api in scsi_bios_ptable()
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_devcie.
Signed-off-by: Yu Kuai
---
drivers/scsi/scsicam.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/scsicam.c b/drivers/scsi/scsicam.c
index e2c7d8ef205f..9617d70c0ed1 100644
--- a/drivers/scsi/scsicam.c
+++ b/drivers/scsi/scsicam.c
@@ -32,11 +32,9 @@
*/
unsigned char *scsi_bios_ptable(struct block_device *dev)
{
- struct address_space *mapping = bdev_whole(dev)->bd_inode->i_mapping;
unsigned char *res = NULL;
- struct folio *folio;
+ struct folio *folio = bdev_read_folio(bdev_whole(dev), 0);
- folio = read_mapping_folio(mapping, 0, NULL);
if (IS_ERR(folio))
return NULL;
--
2.39.2
[PATCH RFC v3 for-6.8/block 08/17] bio: export bio_add_folio_nofail()
From: Yu Kuai Currently btrfs is using __bio_add_page() in write_dev_supers(). In order to convert to use folio for bdev in btrfs, export bio_add_folio_nofail() so that it can replace __bio_add_page(). Signed-off-by: Yu Kuai --- block/bio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/bio.c b/block/bio.c index b9642a41f286..c7459839ca40 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1122,6 +1122,7 @@ void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len, WARN_ON_ONCE(off > UINT_MAX); __bio_add_page(bio, &folio->page, len, off); } +EXPORT_SYMBOL_GPL(bio_add_folio_nofail); /** * bio_add_folio - Attempt to add part of a folio to a bio. -- 2.39.2
[PATCH RFC v3 for-6.8/block 09/17] btrfs: use bdev apis
From: Yu Kuai
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai
---
fs/btrfs/disk-io.c | 71 +-
fs/btrfs/volumes.c | 17 ++-
fs/btrfs/zoned.c | 15 +-
3 files changed, 48 insertions(+), 55 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 401ea09ae4b8..a1cfdee99a81 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3620,28 +3620,24 @@ ALLOW_ERROR_INJECTION(open_ctree, ERRNO);
static void btrfs_end_super_write(struct bio *bio)
{
struct btrfs_device *device = bio->bi_private;
- struct bio_vec *bvec;
- struct bvec_iter_all iter_all;
- struct page *page;
-
- bio_for_each_segment_all(bvec, bio, iter_all) {
- page = bvec->bv_page;
+ struct folio_iter fi;
+ bio_for_each_folio_all(fi, bio) {
if (bio->bi_status) {
btrfs_warn_rl_in_rcu(device->fs_info,
"lost page write due to IO error on %s (%d)",
btrfs_dev_name(device),
blk_status_to_errno(bio->bi_status));
- ClearPageUptodate(page);
- SetPageError(page);
+ folio_clear_uptodate(fi.folio);
+ folio_set_error(fi.folio);
btrfs_dev_stat_inc_and_print(device,
BTRFS_DEV_STAT_WRITE_ERRS);
} else {
- SetPageUptodate(page);
+ folio_mark_uptodate(fi.folio);
}
- put_page(page);
- unlock_page(page);
+ folio_put(fi.folio);
+ folio_unlock(fi.folio);
}
bio_put(bio);
@@ -3651,9 +3647,9 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct
block_device *bdev,
int copy_num, bool
drop_cache)
{
struct btrfs_super_block *super;
- struct page *page;
+ struct folio *folio;
u64 bytenr, bytenr_orig;
- struct address_space *mapping = bdev->bd_inode->i_mapping;
+ unsigned int nofs_flag;
int ret;
bytenr_orig = btrfs_sb_offset(copy_num);
@@ -3674,16 +3670,17 @@ struct btrfs_super_block
*btrfs_read_dev_one_super(struct block_device *bdev,
* Drop the page of the primary superblock, so later read will
* always read from the device.
*/
- invalidate_inode_pages2_range(mapping,
- bytenr >> PAGE_SHIFT,
+ invalidate_bdev_range(bdev, bytenr >> PAGE_SHIFT,
(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
}
- page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
- if (IS_ERR(page))
- return ERR_CAST(page);
+ nofs_flag = memalloc_nofs_save();
+ folio = bdev_read_folio(bdev, bytenr);
+ memalloc_nofs_restore(nofs_flag);
+ if (IS_ERR(folio))
+ return ERR_CAST(folio);
- super = page_address(page);
+ super = folio_address(folio);
if (btrfs_super_magic(super) != BTRFS_MAGIC) {
btrfs_release_disk_super(super);
return ERR_PTR(-ENODATA);
@@ -3740,7 +3737,6 @@ static int write_dev_supers(struct btrfs_device *device,
struct btrfs_super_block *sb, int max_mirrors)
{
struct btrfs_fs_info *fs_info = device->fs_info;
- struct address_space *mapping = device->bdev->bd_inode->i_mapping;
SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
int i;
int errors = 0;
@@ -3753,7 +3749,7 @@ static int write_dev_supers(struct btrfs_device *device,
shash->tfm = fs_info->csum_shash;
for (i = 0; i < max_mirrors; i++) {
- struct page *page;
+ struct folio *folio;
struct bio *bio;
struct btrfs_super_block *disk_super;
@@ -3778,9 +3774,10 @@ static int write_dev_supers(struct btrfs_device *device,
BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE,
sb->csum);
- page = find_or_create_page(mapping, bytenr >> PAGE_SHIFT,
- GFP_NOFS);
- if (!page) {
+ folio = bdev_get_folio(device->bdev, bytenr,
+ FGP_LOCK|FGP_ACCESSED|FGP_CREAT,
+ GFP_NOFS);
+ if (IS_ERR(folio)) {
btrfs_err(device->fs_info,
"couldn't get super block page for bytenr %llu",
bytenr);
@@ -3789,9 +3786,9 @@ static int write_dev_supers(struct b
[PATCH RFC v3 for-6.8/block 11/17] erofs: use bdev api
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai
---
fs/erofs/data.c | 18 --
fs/erofs/internal.h | 2 ++
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index c98aeda8abb2..bbe2fe199bf3 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -32,8 +32,8 @@ void erofs_put_metabuf(struct erofs_buf *buf)
void *erofs_bread(struct erofs_buf *buf, erofs_blk_t blkaddr,
enum erofs_kmap_type type)
{
- struct inode *inode = buf->inode;
- erofs_off_t offset = (erofs_off_t)blkaddr << inode->i_blkbits;
+ u8 blkszbits = buf->inode ? buf->inode->i_blkbits : buf->blkszbits;
+ erofs_off_t offset = (erofs_off_t)blkaddr << blkszbits;
pgoff_t index = offset >> PAGE_SHIFT;
struct page *page = buf->page;
struct folio *folio;
@@ -43,7 +43,9 @@ void *erofs_bread(struct erofs_buf *buf, erofs_blk_t blkaddr,
erofs_put_metabuf(buf);
nofs_flag = memalloc_nofs_save();
- folio = read_cache_folio(inode->i_mapping, index, NULL, NULL);
+ folio = buf->inode ?
+ read_mapping_folio(buf->inode->i_mapping, index, NULL) :
+ bdev_read_folio(buf->bdev, offset);
memalloc_nofs_restore(nofs_flag);
if (IS_ERR(folio))
return folio;
@@ -67,10 +69,14 @@ void *erofs_bread(struct erofs_buf *buf, erofs_blk_t
blkaddr,
void erofs_init_metabuf(struct erofs_buf *buf, struct super_block *sb)
{
- if (erofs_is_fscache_mode(sb))
+ if (erofs_is_fscache_mode(sb)) {
buf->inode = EROFS_SB(sb)->s_fscache->inode;
- else
- buf->inode = sb->s_bdev->bd_inode;
+ buf->bdev = NULL;
+ } else {
+ buf->inode = NULL;
+ buf->bdev = sb->s_bdev;
+ buf->blkszbits = EROFS_SB(sb)->blkszbits;
+ }
}
void *erofs_read_metabuf(struct erofs_buf *buf, struct super_block *sb,
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index b0409badb017..c9206351b485 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -224,8 +224,10 @@ enum erofs_kmap_type {
struct erofs_buf {
struct inode *inode;
+ struct block_device *bdev;
struct page *page;
void *base;
+ u8 blkszbits;
enum erofs_kmap_type kmap_type;
};
#define __EROFS_BUF_INITIALIZER((struct erofs_buf){ .page = NULL })
--
2.39.2
[PATCH RFC v3 for-6.8/block 12/17] nilfs2: use bdev api in nilfs_attach_log_writer()
From: Yu Kuai Avoid to access bd_inode directly, prepare to remove bd_inode from block_device. Signed-off-by: Yu Kuai --- fs/nilfs2/segment.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c index 55e31cc903d1..a1130e384937 100644 --- a/fs/nilfs2/segment.c +++ b/fs/nilfs2/segment.c @@ -2823,7 +2823,7 @@ int nilfs_attach_log_writer(struct super_block *sb, struct nilfs_root *root) if (!nilfs->ns_writer) return -ENOMEM; - inode_attach_wb(nilfs->ns_bdev->bd_inode, NULL); + bdev_attach_wb(nilfs->ns_bdev); err = nilfs_segctor_start_thread(nilfs->ns_writer); if (unlikely(err)) -- 2.39.2
[PATCH RFC v3 for-6.8/block 13/17] jbd2: use bdev apis
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai
---
fs/jbd2/journal.c | 3 +--
fs/jbd2/recovery.c | 6 ++
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index ed53188472f9..f1b5ffeaf02a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2003,8 +2003,7 @@ static int __jbd2_journal_erase(journal_t *journal,
unsigned int flags)
byte_count = (block_stop - block_start + 1) *
journal->j_blocksize;
- truncate_inode_pages_range(journal->j_dev->bd_inode->i_mapping,
- byte_start, byte_stop);
+ truncate_bdev_range(journal->j_dev, 0, byte_start, byte_stop);
if (flags & JBD2_JOURNAL_FLUSH_DISCARD) {
err = blkdev_issue_discard(journal->j_dev,
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 01f744cb97a4..6b6a2c4585fa 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -290,7 +290,6 @@ int jbd2_journal_recover(journal_t *journal)
struct recovery_infoinfo;
errseq_twb_err;
- struct address_space*mapping;
memset(&info, 0, sizeof(info));
sb = journal->j_superblock;
@@ -309,8 +308,7 @@ int jbd2_journal_recover(journal_t *journal)
}
wb_err = 0;
- mapping = journal->j_fs_dev->bd_inode->i_mapping;
- errseq_check_and_advance(&mapping->wb_err, &wb_err);
+ bdev_wb_err_check_and_advance(journal->j_fs_dev, &wb_err);
err = do_one_pass(journal, &info, PASS_SCAN);
if (!err)
err = do_one_pass(journal, &info, PASS_REVOKE);
@@ -334,7 +332,7 @@ int jbd2_journal_recover(journal_t *journal)
err2 = sync_blockdev(journal->j_fs_dev);
if (!err)
err = err2;
- err2 = errseq_check_and_advance(&mapping->wb_err, &wb_err);
+ err2 = bdev_wb_err_check_and_advance(journal->j_fs_dev, &wb_err);
if (!err)
err = err2;
/* Make sure all replayed data is on permanent storage */
--
2.39.2
[PATCH RFC v3 for-6.8/block 15/17] ext4: use new helper to read sb block
From: Yu Kuai
Remove __ext4_sb_bread_gfp() and ext4_buffer_uptodate() that is defined
by ext4, and convert to use common helper __bread_gfp2() and
buffer_uptodate_or_error().
Signed-off-by: Yu Kuai
Reviewed-by: Jan Kara
---
fs/ext4/ext4.h| 13 -
fs/ext4/inode.c | 8
fs/ext4/super.c | 45 ++---
fs/ext4/symlink.c | 2 +-
4 files changed, 15 insertions(+), 53 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a5d784872303..8377f6c5264f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3824,19 +3824,6 @@ extern const struct iomap_ops ext4_iomap_ops;
extern const struct iomap_ops ext4_iomap_overwrite_ops;
extern const struct iomap_ops ext4_iomap_report_ops;
-static inline int ext4_buffer_uptodate(struct buffer_head *bh)
-{
- /*
-* If the buffer has the write error flag, we have failed
-* to write out data in the block. In this case, we don't
-* have to read the block because we may read the old data
-* successfully.
-*/
- if (buffer_write_io_error(bh))
- set_buffer_uptodate(bh);
- return buffer_uptodate(bh);
-}
-
#endif /* __KERNEL__ */
#define EFSBADCRC EBADMSG /* Bad CRC detected */
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 61277f7f8722..efb0af6f02f7 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -887,7 +887,7 @@ struct buffer_head *ext4_bread(handle_t *handle, struct
inode *inode,
bh = ext4_getblk(handle, inode, block, map_flags);
if (IS_ERR(bh))
return bh;
- if (!bh || ext4_buffer_uptodate(bh))
+ if (!bh || buffer_uptodate_or_error(bh))
return bh;
ret = ext4_read_bh_lock(bh, REQ_META | REQ_PRIO, true);
@@ -915,7 +915,7 @@ int ext4_bread_batch(struct inode *inode, ext4_lblk_t
block, int bh_count,
for (i = 0; i < bh_count; i++)
/* Note that NULL bhs[i] is valid because of holes. */
- if (bhs[i] && !ext4_buffer_uptodate(bhs[i]))
+ if (bhs[i] && !buffer_uptodate_or_error(bhs[i]))
ext4_read_bh_lock(bhs[i], REQ_META | REQ_PRIO, false);
if (!wait)
@@ -4392,11 +4392,11 @@ static int __ext4_get_inode_loc(struct super_block *sb,
unsigned long ino,
bh = sb_getblk(sb, block);
if (unlikely(!bh))
return -ENOMEM;
- if (ext4_buffer_uptodate(bh))
+ if (buffer_uptodate_or_error(bh))
goto has_buffer;
lock_buffer(bh);
- if (ext4_buffer_uptodate(bh)) {
+ if (buffer_uptodate_or_error(bh)) {
/* Someone brought it uptodate while we waited */
unlock_buffer(bh);
goto has_buffer;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5fcf377ab1f..3f07eaa2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -180,7 +180,7 @@ void ext4_read_bh_nowait(struct buffer_head *bh, blk_opf_t
op_flags,
{
BUG_ON(!buffer_locked(bh));
- if (ext4_buffer_uptodate(bh)) {
+ if (buffer_uptodate_or_error(bh)) {
unlock_buffer(bh);
return;
}
@@ -191,7 +191,7 @@ int ext4_read_bh(struct buffer_head *bh, blk_opf_t
op_flags, bh_end_io_t *end_io
{
BUG_ON(!buffer_locked(bh));
- if (ext4_buffer_uptodate(bh)) {
+ if (buffer_uptodate_or_error(bh)) {
unlock_buffer(bh);
return 0;
}
@@ -214,49 +214,24 @@ int ext4_read_bh_lock(struct buffer_head *bh, blk_opf_t
op_flags, bool wait)
return ext4_read_bh(bh, op_flags, NULL);
}
-/*
- * This works like __bread_gfp() except it uses ERR_PTR for error
- * returns. Currently with sb_bread it's impossible to distinguish
- * between ENOMEM and EIO situations (since both result in a NULL
- * return.
- */
-static struct buffer_head *__ext4_sb_bread_gfp(struct super_block *sb,
- sector_t block,
- blk_opf_t op_flags, gfp_t gfp)
-{
- struct buffer_head *bh;
- int ret;
-
- bh = sb_getblk_gfp(sb, block, gfp);
- if (bh == NULL)
- return ERR_PTR(-ENOMEM);
- if (ext4_buffer_uptodate(bh))
- return bh;
-
- ret = ext4_read_bh_lock(bh, REQ_META | op_flags, true);
- if (ret) {
- put_bh(bh);
- return ERR_PTR(ret);
- }
- return bh;
-}
-
struct buffer_head *ext4_sb_bread(struct super_block *sb, sector_t block,
blk_opf_t op_flags)
{
- gfp_t gfp = mapping_gfp_constraint(sb->s_bdev->bd_inode->i_mapping,
- ~__GFP_FS) | __GFP_MOVABLE;
+ struct buffer_head *bh = __bread_gfp2(sb->s_bdev, block,
+ sb->s_blocksize,
+ REQ_META | op_flags,
+
[PATCH RFC v3 for-6.8/block 14/17] buffer: add a new helper to read sb block
From: Yu Kuai
Unlike __bread_gfp(), ext4 has special handing while reading sb block:
1) __GFP_NOFAIL is not set, and memory allocation can fail;
2) If buffer write failed before, set buffer uptodate and don't read
block from disk;
3) REQ_META is set for all IO, and REQ_PRIO is set for reading xattr;
4) If failed, return error ptr instead of NULL;
This patch add a new helper __bread_gfp2() that will match above 2 and 3(
1 will be used, and 4 will still be encapsulated by ext4), and prepare to
prevent calling mapping_gfp_constraint() directly on bd_inode->i_mapping
in ext4.
Signed-off-by: Yu Kuai
---
fs/buffer.c | 68 ++---
include/linux/buffer_head.h | 18 +-
2 files changed, 65 insertions(+), 21 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 967f34b70aa8..188bd36c9fea 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1255,16 +1255,19 @@ void __bforget(struct buffer_head *bh)
}
EXPORT_SYMBOL(__bforget);
-static struct buffer_head *__bread_slow(struct buffer_head *bh)
+static struct buffer_head *__bread_slow(struct buffer_head *bh,
+ blk_opf_t op_flags,
+ bool check_write_error)
{
lock_buffer(bh);
- if (buffer_uptodate(bh)) {
+ if (buffer_uptodate(bh) ||
+ (check_write_error && buffer_uptodate_or_error(bh))) {
unlock_buffer(bh);
return bh;
} else {
get_bh(bh);
bh->b_end_io = end_buffer_read_sync;
- submit_bh(REQ_OP_READ, bh);
+ submit_bh(REQ_OP_READ | op_flags, bh);
wait_on_buffer(bh);
if (buffer_uptodate(bh))
return bh;
@@ -1445,6 +1448,31 @@ void __breadahead(struct block_device *bdev, sector_t
block, unsigned size)
}
EXPORT_SYMBOL(__breadahead);
+static struct buffer_head *
+bread_gfp(struct block_device *bdev, sector_t block, unsigned int size,
+ blk_opf_t op_flags, gfp_t gfp, bool check_write_error)
+{
+ struct buffer_head *bh;
+
+ gfp |= mapping_gfp_constraint(bdev->bd_inode->i_mapping, ~__GFP_FS);
+
+ /*
+* Prefer looping in the allocator rather than here, at least that
+* code knows what it's doing.
+*/
+ gfp |= __GFP_NOFAIL;
+
+ bh = bdev_getblk(bdev, block, size, gfp);
+ if (unlikely(!bh))
+ return NULL;
+
+ if (buffer_uptodate(bh) ||
+ (check_write_error && buffer_uptodate_or_error(bh)))
+ return bh;
+
+ return __bread_slow(bh, op_flags, check_write_error);
+}
+
/**
* __bread_gfp() - reads a specified block and returns the bh
* @bdev: the block_device to read from
@@ -1458,27 +1486,27 @@ EXPORT_SYMBOL(__breadahead);
* It returns NULL if the block was unreadable.
*/
struct buffer_head *
-__bread_gfp(struct block_device *bdev, sector_t block,
- unsigned size, gfp_t gfp)
+__bread_gfp(struct block_device *bdev, sector_t block, unsigned int size,
+ gfp_t gfp)
{
- struct buffer_head *bh;
-
- gfp |= mapping_gfp_constraint(bdev->bd_inode->i_mapping, ~__GFP_FS);
-
- /*
-* Prefer looping in the allocator rather than here, at least that
-* code knows what it's doing.
-*/
- gfp |= __GFP_NOFAIL;
-
- bh = bdev_getblk(bdev, block, size, gfp);
-
- if (likely(bh) && !buffer_uptodate(bh))
- bh = __bread_slow(bh);
- return bh;
+ return bread_gfp(bdev, block, size, 0, gfp, false);
}
EXPORT_SYMBOL(__bread_gfp);
+/*
+ * This works like __bread_gfp() except:
+ * 1) If buffer write failed before, set buffer uptodate and don't read
+ * block from disk;
+ * 2) Caller can pass in additional op_flags like REQ_META;
+ */
+struct buffer_head *
+__bread_gfp2(struct block_device *bdev, sector_t block, unsigned int size,
+blk_opf_t op_flags, gfp_t gfp)
+{
+ return bread_gfp(bdev, block, size, op_flags, gfp, true);
+}
+EXPORT_SYMBOL(__bread_gfp2);
+
static void __invalidate_bh_lrus(struct bh_lru *b)
{
int i;
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 5f23ee599889..751b2744b4ae 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -171,6 +171,18 @@ static __always_inline int buffer_uptodate(const struct
buffer_head *bh)
return test_bit_acquire(BH_Uptodate, &bh->b_state);
}
+static __always_inline int buffer_uptodate_or_error(struct buffer_head *bh)
+{
+ /*
+* If the buffer has the write error flag, data was failed to write
+* out in the block. In this case, set buffer uptodate to prevent
+* reading old data.
+*/
+ if (buffer_write_io_error(bh))
+ set_buffer_uptodate(bh);
+ return buffer_uptodate(bh);
+}
+
static inline unsigned long bh_offset(const struct buffer_head *bh)
{
retu
[PATCH RFC v3 for-6.8/block 16/17] ext4: remove block_device_ejected()
From: Yu Kuai
block_device_ejected() is added by commit bdfe0cbd746a ("Revert
"ext4: remove block_device_ejected"") in 2015. At that time 'bdi->wb'
is destroyed synchronized from del_gendisk(), hence if ext4 is still
mounted, and then mark_buffer_dirty() will reference destroyed 'wb'.
However, such problem doesn't exist anymore:
- commit d03f6cdc1fc4 ("block: Dynamically allocate and refcount
backing_dev_info") switch bdi to use refcounting;
- commit 13eec2363ef0 ("fs: Get proper reference for s_bdi"), will grab
additional reference of bdi while mounting, so that 'bdi->wb' will not
be destroyed until generic_shutdown_super().
Hence remove this dead function block_device_ejected().
Signed-off-by: Yu Kuai
Reviewed-by: Jan Kara
Reviewed-by: Christoph Hellwig
---
fs/ext4/super.c | 18 --
1 file changed, 18 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3f07eaa2..a7935edbd7b1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -467,22 +467,6 @@ static void ext4_maybe_update_superblock(struct
super_block *sb)
schedule_work(&EXT4_SB(sb)->s_sb_upd_work);
}
-/*
- * The del_gendisk() function uninitializes the disk-specific data
- * structures, including the bdi structure, without telling anyone
- * else. Once this happens, any attempt to call mark_buffer_dirty()
- * (for example, by ext4_commit_super), will cause a kernel OOPS.
- * This is a kludge to prevent these oops until we can put in a proper
- * hook in del_gendisk() to inform the VFS and file system layers.
- */
-static int block_device_ejected(struct super_block *sb)
-{
- struct inode *bd_inode = sb->s_bdev->bd_inode;
- struct backing_dev_info *bdi = inode_to_bdi(bd_inode);
-
- return bdi->dev == NULL;
-}
-
static void ext4_journal_commit_callback(journal_t *journal, transaction_t
*txn)
{
struct super_block *sb = journal->j_private;
@@ -6162,8 +6146,6 @@ static int ext4_commit_super(struct super_block *sb)
if (!sbh)
return -EINVAL;
- if (block_device_ejected(sb))
- return -ENODEV;
ext4_update_super(sb);
--
2.39.2
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
On 20.12.2023 17:34, Sébastien Chaumat wrote: > Here are the patches I made to xen and linux kernel > Plus dmesg (bare metal,xen) and "xl dmesg" So only a single setup_gsi then, after all. Or else I must be missing something. And there it's all consistent as well: Level/low like for any other pin (IO-APIC) based PCI IRQ. The issue may then rather be with xen_register_pirq(), where 1st and 2nd invocation are clearly different. I'll try to get to looking into that some more later in the day. Jan
[PATCH RFC v3 for-6.8/block 17/17] ext4: use bdev apis
From: Yu Kuai
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai
Reviewed-by: Jan Kara
---
fs/ext4/dir.c | 6 ++
fs/ext4/ext4_jbd2.c | 6 +++---
fs/ext4/super.c | 3 +--
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 3985f8c33f95..64e35eb6a324 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -191,10 +191,8 @@ static int ext4_readdir(struct file *file, struct
dir_context *ctx)
pgoff_t index = map.m_pblk >>
(PAGE_SHIFT - inode->i_blkbits);
if (!ra_has_index(&file->f_ra, index))
- page_cache_sync_readahead(
- sb->s_bdev->bd_inode->i_mapping,
- &file->f_ra, file,
- index, 1);
+ bdev_sync_readahead(sb->s_bdev, &file->f_ra,
+ file, index, 1);
file->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT;
bh = ext4_bread(NULL, inode, map.m_lblk, 0);
if (IS_ERR(bh)) {
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index d1a2e6624401..c1bf3a00fad9 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -206,7 +206,6 @@ static void ext4_journal_abort_handle(const char *caller,
unsigned int line,
static void ext4_check_bdev_write_error(struct super_block *sb)
{
- struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
struct ext4_sb_info *sbi = EXT4_SB(sb);
int err;
@@ -216,9 +215,10 @@ static void ext4_check_bdev_write_error(struct super_block
*sb)
* we could read old data from disk and write it out again, which
* may lead to on-disk filesystem inconsistency.
*/
- if (errseq_check(&mapping->wb_err, READ_ONCE(sbi->s_bdev_wb_err))) {
+ if (bdev_wb_err_check(sb->s_bdev, READ_ONCE(sbi->s_bdev_wb_err))) {
spin_lock(&sbi->s_bdev_wb_lock);
- err = errseq_check_and_advance(&mapping->wb_err,
&sbi->s_bdev_wb_err);
+ err = bdev_wb_err_check_and_advance(sb->s_bdev,
+ &sbi->s_bdev_wb_err);
spin_unlock(&sbi->s_bdev_wb_lock);
if (err)
ext4_error_err(sb, -err,
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a7935edbd7b1..25c3d2ac8559 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5544,8 +5544,7 @@ static int __ext4_fill_super(struct fs_context *fc,
struct super_block *sb)
* used to detect the metadata async write error.
*/
spin_lock_init(&sbi->s_bdev_wb_lock);
- errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
-&sbi->s_bdev_wb_err);
+ bdev_wb_err_check_and_advance(sb->s_bdev, &sbi->s_bdev_wb_err);
EXT4_SB(sb)->s_mount_state |= EXT4_ORPHAN_FS;
ext4_orphan_cleanup(sb, es);
EXT4_SB(sb)->s_mount_state &= ~EXT4_ORPHAN_FS;
--
2.39.2
Re: [PATCH 0/2] xen: have a more generic unaligned.h header (take 2)
On 12.12.23 17:27, Juergen Gross wrote: Second try for the generic unaligned.h approach. This time including a fix for building stubdom with libxenguest, which is using a cruel hack to reuse the hypervisor's decompressing code. Juergen Gross (2): xen: make include/xen/unaligned.h usable on all architectures xen: remove asm/unaligned.h .../guest/xg_dom_decompress_unsafe_zstd.c | 2 +- xen/arch/x86/include/asm/unaligned.h | 6 --- xen/common/lz4/defs.h | 2 +- xen/common/lzo.c | 2 +- xen/common/unlzo.c| 2 +- xen/common/xz/private.h | 2 +- xen/common/zstd/mem.h | 2 +- xen/include/xen/unaligned.h | 53 +++ xen/lib/xxhash32.c| 2 +- xen/lib/xxhash64.c| 2 +- 10 files changed, 38 insertions(+), 37 deletions(-) delete mode 100644 xen/arch/x86/include/asm/unaligned.h Is anything missing for this series to go in? Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [PATCH 0/2] xen: have a more generic unaligned.h header (take 2)
On 21/12/2023 9:37 am, Juergen Gross wrote: > On 12.12.23 17:27, Juergen Gross wrote: >> Second try for the generic unaligned.h approach. >> >> This time including a fix for building stubdom with libxenguest, >> which is using a cruel hack to reuse the hypervisor's decompressing >> code. >> >> Juergen Gross (2): >> xen: make include/xen/unaligned.h usable on all architectures >> xen: remove asm/unaligned.h >> >> .../guest/xg_dom_decompress_unsafe_zstd.c | 2 +- >> xen/arch/x86/include/asm/unaligned.h | 6 --- >> xen/common/lz4/defs.h | 2 +- >> xen/common/lzo.c | 2 +- >> xen/common/unlzo.c | 2 +- >> xen/common/xz/private.h | 2 +- >> xen/common/zstd/mem.h | 2 +- >> xen/include/xen/unaligned.h | 53 +++ >> xen/lib/xxhash32.c | 2 +- >> xen/lib/xxhash64.c | 2 +- >> 10 files changed, 38 insertions(+), 37 deletions(-) >> delete mode 100644 xen/arch/x86/include/asm/unaligned.h >> > > Is anything missing for this series to go in? Oh - I'd not spotted it. Lemme throw it through a full Gitlab CI run just to be sure. ~Andrew
Re: [PATCH v2 07/39] xen/riscv: introduce arch-riscv/hvm/save.h
On Thu, 2023-12-21 at 08:58 +0100, Jan Beulich wrote: > On 20.12.2023 21:05, Oleksii wrote: > > On Tue, 2023-12-05 at 16:59 +0100, Jan Beulich wrote: > > > On 24.11.2023 11:30, Oleksii Kurochko wrote: > > > > --- /dev/null > > > > +++ b/xen/include/public/arch-riscv/hvm/save.h > > > > @@ -0,0 +1,20 @@ > > > > +/* SPDX-License-Identifier: MIT */ > > > > +/* > > > > + * Structure definitions for HVM state that is held by Xen and > > > > must > > > > + * be saved along with the domain's memory and device-model > > > > state. > > > > + */ > > > > + > > > > +#ifndef __XEN_PUBLIC_HVM_SAVE_RISCV_H__ > > > > +#define __XEN_PUBLIC_HVM_SAVE_RISCV_H__ > > > > + > > > > +#endif > > > > + > > > > +/* > > > > + * Local variables: > > > > + * mode: C > > > > + * c-file-style: "BSD" > > > > + * c-basic-offset: 4 > > > > + * tab-width: 4 > > > > + * indent-tabs-mode: nil > > > > + * End: > > > > + */ > > > > > > Seeing that Arm's is as empty, I wonder why we have it. Julien, > > > Stefano? > > I'm still curious about the reason here, but ... > > > It seems to exist to satisfy the 'install-tools-public-headers' > > target: > > install: cannot stat 'xen/arch-arm/hvm/*.h': No such file or > > directory > > Makefile:58: recipe for target 'install' failed > > make[1]: *** [install] Error 1 > > make[1]: Leaving directory '/builds/xen- > > project/people/olkur/xen/tools/include' > > Makefile:44: recipe for target 'install-tools-public-headers' > > failed > > > > From tools/include/Makefile: > > install: all > > ... > > $(DESTDIR)$(includedir)/xen/arch-arm > > $(INSTALL_DATA) xen/arch-arm/hvm/*.h > > $(DESTDIR)$(includedir)/xen/arch-arm/hvm > > ... > > > > We have the following options: > > 1. Remove the line with $(INSTALL_DATA) xen/arch-arm/hvm/*.h (only > > save.h is now in this folder, which is empty). > > ... we can't easily remove any existing public header. We can only > try to > avoid making the same mistake (even if it's just a minor one) again. > > > 2. Don't touch the Arm part, but for PPC and RISC-V, do the > > following: > > #if defined(__i386__) || defined(__x86_64__) > > #include "../arch-x86/hvm/save.h" > > #elif defined(__arm__) || defined(__aarch64__) > > #include "../arch-arm/hvm/save.h" > > +#elif defined(__powerpc64__) || defined(__riscv) > > +/* no specific header to include */ > > #else > > #error "unsupported architecture" > > #endif > > Yes. Still awaiting Shawn's input here as well, though. Perhaps you missed the email from Shawn: https://lore.kernel.org/xen-devel/c2f3280e-2208-496b-a0b5-fda1a2076...@raptorengineering.com/ > > > 3. Provide an asm-generic version of save.h for Arm, PPC, and RISC- > > V > > and use it in public/save.h. > > That's not an option imo - what's under public/ needs to be self- > contained. > Stuff there isn't supposed to even know of asm-generic/. In this case, this is not an option. ~ Oleksii
[PATCH v2] x86/amd: extend CPU errata #1474 affected models
Errata #1474has now been extended to cover models from family 17h ranges
00-2Fh, so the errata now covers all the models released under Family 17h (Zen,
Zen+ and Zen2).
Additionally extend the workaround to Family 18h (Hygon), since it's based on
the Zen architecture and very likely affected.
Rename all the zen2 related symbols to fam17, since the errata doesn't
exclusively affect Zen2 anymore.
Reported-by: Andrew Cooper
Signed-off-by: Roger Pau Monné
Reviewed-by: Andrew Cooper
---
Changes since v1:
- Use fam17 instead of zen prefix.
---
xen/arch/x86/cpu/amd.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 0f305312ff2a..d43288ae9779 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -54,7 +54,7 @@ bool __read_mostly amd_acpi_c1e_quirk;
bool __ro_after_init amd_legacy_ssbd;
bool __initdata amd_virt_spec_ctrl;
-static bool __read_mostly zen2_c6_disabled;
+static bool __read_mostly fam17_c6_disabled;
static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
unsigned int *hi)
@@ -978,24 +978,24 @@ void amd_check_zenbleed(void)
val & chickenbit ? "chickenbit" : "microcode");
}
-static void cf_check zen2_disable_c6(void *arg)
+static void cf_check fam17_disable_c6(void *arg)
{
/* Disable C6 by clearing the CCR{0,1,2}_CC6EN bits. */
const uint64_t mask = ~((1ul << 6) | (1ul << 14) | (1ul << 22));
uint64_t val;
- if (!zen2_c6_disabled) {
+ if (!fam17_c6_disabled) {
printk(XENLOG_WARNING
"Disabling C6 after 1000 days apparent uptime due to AMD errata 1474\n");
- zen2_c6_disabled = true;
+ fam17_c6_disabled = true;
/*
* Prevent CPU hotplug so that started CPUs will either see
-* zen2_c6_disabled set, or will be handled by
+* zen_c6_disabled set, or will be handled by
* smp_call_function().
*/
while (!get_cpu_maps())
process_pending_softirqs();
- smp_call_function(zen2_disable_c6, NULL, 0);
+ smp_call_function(fam17_disable_c6, NULL, 0);
put_cpu_maps();
}
@@ -1294,8 +1294,8 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
amd_check_zenbleed();
amd_check_erratum_1485();
- if (zen2_c6_disabled)
- zen2_disable_c6(NULL);
+ if (fam17_c6_disabled)
+ fam17_disable_c6(NULL);
check_syscfg_dram_mod_en();
@@ -1307,7 +1307,7 @@ const struct cpu_dev amd_cpu_dev = {
.c_init = init_amd,
};
-static int __init cf_check zen2_c6_errata_check(void)
+static int __init cf_check amd_check_erratum_1474(void)
{
/*
* Errata #1474: A Core May Hang After About 1044 Days
@@ -1315,7 +1315,8 @@ static int __init cf_check zen2_c6_errata_check(void)
*/
s_time_t delta;
- if (cpu_has_hypervisor || boot_cpu_data.x86 != 0x17 || !is_zen2_uarch())
+ if (cpu_has_hypervisor ||
+ (boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18))
return 0;
/*
@@ -1330,10 +1331,10 @@ static int __init cf_check zen2_c6_errata_check(void)
if (delta > 0) {
static struct timer errata_c6;
- init_timer(&errata_c6, zen2_disable_c6, NULL, 0);
+ init_timer(&errata_c6, fam17_disable_c6, NULL, 0);
set_timer(&errata_c6, NOW() + delta);
} else
- zen2_disable_c6(NULL);
+ fam17_disable_c6(NULL);
return 0;
}
@@ -1341,4 +1342,4 @@ static int __init cf_check zen2_c6_errata_check(void)
* Must be executed after early_time_init() for tsc_ticks2ns() to have been
* calibrated. That prevents us doing the check in init_amd().
*/
-presmp_initcall(zen2_c6_errata_check);
+presmp_initcall(amd_check_erratum_1474);
--
2.43.0
[PATCH] x86/iommu: add CHANGELOG entry for hwdom setup time improvements
Signed-off-by: Roger Pau Monné --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee5d41fc933..52484c047bd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Changed - Changed flexible array definitions in public I/O interface headers to not use "1" as the number of array elements. + - On x86: + - Reduce IOMMU setup time for hardware domain. ### Added - On x86: -- 2.43.0
Re: [PATCH v2] x86/amd: extend CPU errata #1474 affected models
On 21/12/2023 10:08 am, Roger Pau Monne wrote: > Errata #1474has now been extended to cover models from family 17h ranges Extra space needed. Can be fixed on commit. > 00-2Fh, so the errata now covers all the models released under Family 17h > (Zen, > Zen+ and Zen2). > > Additionally extend the workaround to Family 18h (Hygon), since it's based on > the Zen architecture and very likely affected. > > Rename all the zen2 related symbols to fam17, since the errata doesn't > exclusively affect Zen2 anymore. > > Reported-by: Andrew Cooper > Signed-off-by: Roger Pau Monné > Reviewed-by: Andrew Cooper ~Andrew
Re: [PATCH v2] x86/amd: extend CPU errata #1474 affected models
On 21.12.2023 11:17, Andrew Cooper wrote: > On 21/12/2023 10:08 am, Roger Pau Monne wrote: >> Errata #1474has now been extended to cover models from family 17h ranges > > Extra space needed. Can be fixed on commit. Also (not just here) - isn't it "erratum" when we talk of just one? Jan >> 00-2Fh, so the errata now covers all the models released under Family 17h >> (Zen, >> Zen+ and Zen2). >> >> Additionally extend the workaround to Family 18h (Hygon), since it's based on >> the Zen architecture and very likely affected. >> >> Rename all the zen2 related symbols to fam17, since the errata doesn't >> exclusively affect Zen2 anymore. >> >> Reported-by: Andrew Cooper >> Signed-off-by: Roger Pau Monné >> Reviewed-by: Andrew Cooper > > ~Andrew
Re: [PATCH] x86/iommu: add CHANGELOG entry for hwdom setup time improvements
On 21.12.2023 11:12, Roger Pau Monne wrote: > Signed-off-by: Roger Pau Monné > --- > CHANGELOG.md | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 5ee5d41fc933..52484c047bd1 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -9,6 +9,8 @@ The format is based on [Keep a > Changelog](https://keepachangelog.com/en/1.0.0/) > ### Changed > - Changed flexible array definitions in public I/O interface headers to not > use "1" as the number of array elements. > + - On x86: > + - Reduce IOMMU setup time for hardware domain. > > ### Added > - On x86: I'm a little puzzled: Isn't this more like patch 7/6 of the v4 series (or possibly patch 5.5/6), which hasn't gone in yet? Jan
Re: [PATCH v2] x86/amd: extend CPU errata #1474 affected models
On 21/12/2023 10:20 am, Jan Beulich wrote: > On 21.12.2023 11:17, Andrew Cooper wrote: >> On 21/12/2023 10:08 am, Roger Pau Monne wrote: >>> Errata #1474has now been extended to cover models from family 17h ranges >> Extra space needed. Can be fixed on commit. > Also (not just here) - isn't it "erratum" when we talk of just one? Good point, yes. I'll fix both. (I'm just collecting some patches for a Gitlab CI run on Juergen's patches, and I'll include this one.) ~Andrew
[PATCH v2 0/1] Implementation of resource_query_layout
Hi all, Sorry to late reply. This is v2 of the implementation of resource_query_layout. This adds a new ioctl to let guest query information of host resource, which is originally from Daniel Stone. We add some changes to support query the correct stride of host resource before it's created, which is to support dGPU prime feature. Without correct stride, dGPU can not blit data to virtio iGPU correctly. Changes from v1 to v2: -Squash two patches to a single patch. -A small modification of VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT Below is description of v1: This add implementation of resource_query_layout to get the information of how the host has actually allocated the buffer. This function is now used to query the stride for guest linear resource for dGPU prime on guest VMs. v1 of qemu side: https: //lore.kernel.org/qemu-devel/20231110074027.24862-1-julia.zh...@amd.com/T/#t v1 of kernel side: https: //lore.kernel.org/xen-devel/20231110074027.24862-1-julia.zh...@amd.com/T/#t Daniel Stone (1): virgl: Implement resource_query_layout hw/display/virtio-gpu-base.c| 4 +++ hw/display/virtio-gpu-virgl.c | 40 + include/hw/virtio/virtio-gpu-bswap.h| 7 include/standard-headers/linux/virtio_gpu.h | 30 meson.build | 4 +++ 5 files changed, 85 insertions(+) -- 2.34.1
[PATCH v2 1/1] virgl: Implement resource_query_layout
From: Daniel Stone
A new ioctl to shuttle information between host and guest about the
actual buffer allocation, which can be used for interop between GL and
Vulkan when supporting standard window systems.
Signed-off-by: Daniel Stone
Co-developed-by: Julia Zhang
Signed-off-by: Julia Zhang
---
hw/display/virtio-gpu-base.c| 4 +++
hw/display/virtio-gpu-virgl.c | 40 +
include/hw/virtio/virtio-gpu-bswap.h| 7
include/standard-headers/linux/virtio_gpu.h | 30
meson.build | 4 +++
5 files changed, 85 insertions(+)
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 6bcee3882f..09b37f015d 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -240,6 +240,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t
features,
features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
}
+#ifdef HAVE_VIRGL_RESOURCE_QUERY_LAYOUT
+features |= (1 << VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT);
+#endif /* HAVE_VIRGL_RESOURCE_QUERY_LAYOUT */
+
return features;
}
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index c1e7c6d0c6..b331232fee 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -813,6 +813,40 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
#endif /* HAVE_VIRGL_RESOURCE_BLOB */
+#ifdef HAVE_VIRGL_RESOURCE_QUERY_LAYOUT
+static void virgl_cmd_resource_query_layout(VirtIOGPU *g,
+ struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_resource_query_layout qlayout;
+struct virtio_gpu_resp_resource_layout resp;
+struct virgl_renderer_resource_layout rlayout;
+int ret;
+int i;
+VIRTIO_GPU_FILL_CMD(qlayout);
+virtio_gpu_resource_query_layout_bswap(&qlayout);
+
+ret = virgl_renderer_resource_query_layout(qlayout.resource_id, &rlayout,
+ qlayout.width, qlayout.height,
+ qlayout.format, qlayout.bind);
+if (ret != 0) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource %d is not
externally-allocated\n",
+ __func__, qlayout.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}
+
+memset(&resp, 0, sizeof(resp));
+resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT;
+resp.num_planes = rlayout.num_planes;
+resp.modifier = rlayout.modifier;
+for (i = 0; i < resp.num_planes; i++) {
+resp.planes[i].offset = rlayout.planes[i].offset;
+resp.planes[i].stride = rlayout.planes[i].stride;
+}
+virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+}
+#endif /* HAVE_VIRGL_RESOURCE_QUERY_LAYOUT */
+
void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
{
@@ -896,6 +930,12 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
virgl_cmd_set_scanout_blob(g, cmd);
break;
#endif /* HAVE_VIRGL_RESOURCE_BLOB */
+
+#ifdef HAVE_VIRGL_RESOURCE_QUERY_LAYOUT
+case VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT:
+ virgl_cmd_resource_query_layout(g, cmd);
+ break;
+#endif /* HAVE_VIRGL_RESOURCE_QUERY_LAYOUT*/
default:
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
break;
diff --git a/include/hw/virtio/virtio-gpu-bswap.h
b/include/hw/virtio/virtio-gpu-bswap.h
index dd1975e2d4..dea8cf6fd3 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -92,4 +92,11 @@ virtio_gpu_scanout_blob_bswap(struct
virtio_gpu_set_scanout_blob *ssb)
le32_to_cpus(&ssb->offsets[3]);
}
+static inline void
+virtio_gpu_resource_query_layout_bswap(struct virtio_gpu_resource_query_layout
*rql)
+{
+virtio_gpu_ctrl_hdr_bswap(&rql->hdr);
+le32_to_cpus(&rql->resource_id);
+}
+
#endif
diff --git a/include/standard-headers/linux/virtio_gpu.h
b/include/standard-headers/linux/virtio_gpu.h
index c621389f3d..c9a2f58237 100644
--- a/include/standard-headers/linux/virtio_gpu.h
+++ b/include/standard-headers/linux/virtio_gpu.h
@@ -65,6 +65,11 @@
*/
#define VIRTIO_GPU_F_CONTEXT_INIT4
+/*
+ * VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT
+ */
+#define VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT 5
+
enum virtio_gpu_ctrl_type {
VIRTIO_GPU_UNDEFINED = 0,
@@ -95,6 +100,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_SUBMIT_3D,
VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB,
VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB,
+ VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT,
/* cursor commands */
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
@@ -108,6 +114,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_RESP_OK_EDID,
VIRTIO_GPU_RESP_OK_RESOURCE_UUID,
VIRTIO_GPU_RESP_OK_MAP_INFO,
+ VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT,
/* error responses */
VIRT
Re: [PATCH] xen/xmalloc: XMEM_POOL_POISON improvements
On 20/12/2023 9:51 pm, Julien Grall wrote: > Hi Andrew, > > On 20/12/2023 21:47, Andrew Cooper wrote: >> When in use, the spew: >> >> (XEN) Assertion '!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, >> POISON_BYTE, (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)' failed at >> common/xmalloc_tlsf.c:246 >> >> is unweidly and likely meaningless to non-Xen developers. Therefore: >> >> * Switch to IS_ENABLED(). There's no need for full #ifdef-ary. >> * Pull memchr_inv() out into the if(), and provide an error message >> which >> clearly states that corruption has been found. >> >> Signed-off-by: Andrew Cooper > > With one remark: > > Reviewed-by: Julien Grall Thanks. Will fix. ~Andrew
Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
On 20/12/23 22:23, Andrew Cooper wrote: On 20/12/2023 6:24 pm, Stefano Stabellini wrote: On Wed, 20 Dec 2023, Federico Serafini wrote: On 20/12/23 12:55, Jan Beulich wrote: On 20.12.2023 12:48, Julien Grall wrote: On 20/12/2023 11:03, Federico Serafini wrote: --- a/xen/arch/arm/arm64/vsysreg.c +++ b/xen/arch/arm/arm64/vsysreg.c @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, /* RO at EL0. RAZ/WI at EL1 */ if ( regs_mode_is_user(regs) ) return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); -else -return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); + +return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); I don't 100% like this change (mostly because I find if/else clearer here). While (it doesn't matter here) my view on this is different, I'm still puzzled why the tool would complain / why a change here is necessary. It is not _one_ return statement, but there's still (and obviously) no way of falling through. The tool is configurable: if you prefer deviate these cases instead of refactoring the code I can update the configuration. If you say "deviation", it implies that there is a violation. I think Jan's point was that both code versions shouldn't be any different. # option-1 if (a) return f1(); else return f2(); # option-2 if (a) return f1(); return f2(); Both options are equally guaranteed to always return. It looks like a peculiar limitation to only recognize option-2 as something that returns 100% of the times. Also option-1 returns 100% of the times. What am I missing? I don't think this is necessarily a limitation because it highlights cases where an if-else could be replaced with a simple if: some may find an if-else clearer, other may find the single if clearer. From a safety point of view both options are safe because there is no risk of unintentional fall through. If you all agree on the fact that small code refactoring like the ones I proposed are counterproductive, then I can update the tool configuration to consider also option-1 as safe. For completeness, it's worth saying that there is an option-3: return a ? f1() : f2(); which is very clearly only a single return, but I personally don't like this as I often find it to be less clear than either other option. Option-3 is currently considered as safe. All options have a guaranteed return, but there cases including this one where option-1 is clearest way to write the logic. ~Andrew -- Federico Serafini, M.Sc. Software Engineer, BUGSENG (http://bugseng.com)
Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3
On 20/12/23 22:35, Andrew Cooper wrote:
On 20/12/2023 11:03 am, Federico Serafini wrote:
This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
code. No fucntional changes are introduced.
Federico Serafini (7):
xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
xen/arch/arm/arm64/vsysreg.c | 4 ++--
xen/arch/arm/gic-v3.c | 30 +++
xen/arch/arm/guest_walk.c | 4
xen/arch/arm/mem_access.c | 12 +--
xen/arch/arm/mmu/p2m.c| 1 +
xen/arch/arm/traps.c | 18
xen/arch/arm/vcpreg.c | 4 ++--
xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
8 files changed, 61 insertions(+), 14 deletions(-)
Just a couple of notes on style. This isn't a request to change
anything in this series, particularly as most is already committed, but
bear it in mind for what I expect will be similar patches in other areas.
We explicitly permit tabulation when it aids readibility, so patch 2
could have been written:
switch ( hypercall_args[*nr] ) {
case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
case 1: /* Don't clobber x0/r0 -- it's the return value */
case 0: /* -ENOSYS case */
break;
default: BUG();
}
(give or take the brace placement other style issue) We also have cases
where a break before a new case statement is preferred, i.e.:
...
break;
case ...:
This is to prevent larger switch statements from being a straight wall
of text.
If in doubt, match the style around it. Please don't de-tabulate
examples which are already tabulated. (i.e. don't de-tabulate the x86
versions of patch 2.)
~Andrew
Understood, thank you.
--
Federico Serafini, M.Sc.
Software Engineer, BUGSENG (http://bugseng.com)
Re: [PATCH v4 03/12] xen/spinlock: introduce new type for recursive spinlocks
On 12.12.2023 13:57, Julien Grall wrote:
> On 12/12/2023 09:47, Juergen Gross wrote:
>> @@ -109,12 +109,16 @@ struct lock_profile_qhead {
>> spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);
>>\
>> static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);
>>\
>> _LOCK_PROFILE_PTR(l)
>> +#define DEFINE_RSPINLOCK(l)
>> \
>> +rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);
>> \
>> +static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);
>> \
>> +_LOCK_PROFILE_PTR(l)
>>
>> -#define spin_lock_init_prof(s, l)
>> \
>> +#define __spin_lock_init_prof(s, l, locktype)
>> \
>
> If I am not mistaken the double-underscore prefix is a violation in
> MISRA. So can this be renamed to:
>
> spin_lock_init_prof__()?
Is the new parameter needed at all? Can't we use typeof((s)->l) in place of
passing the type explicitly?
Jan
Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
On 21/12/2023 10:29 am, Federico Serafini wrote: > On 20/12/23 22:23, Andrew Cooper wrote: >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote: >>> On Wed, 20 Dec 2023, Federico Serafini wrote: On 20/12/23 12:55, Jan Beulich wrote: > On 20.12.2023 12:48, Julien Grall wrote: >> On 20/12/2023 11:03, Federico Serafini wrote: >>> --- a/xen/arch/arm/arm64/vsysreg.c >>> +++ b/xen/arch/arm/arm64/vsysreg.c >>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>> /* RO at EL0. RAZ/WI at EL1 */ >>> if ( regs_mode_is_user(regs) ) >>> return handle_ro_raz(regs, regidx, >>> hsr.sysreg.read, hsr, >>> 0); >>> - else >>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, >>> hsr, >>> 1); >>> + >>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, >>> hsr, 1); >> I don't 100% like this change (mostly because I find if/else clearer >> here). > While (it doesn't matter here) my view on this is different, I'm > still > puzzled why the tool would complain / why a change here is necessary. > It is not _one_ return statement, but there's still (and > obviously) no > way of falling through. The tool is configurable: if you prefer deviate these cases instead of refactoring the code I can update the configuration. >>> If you say "deviation", it implies that there is a violation. I think >>> Jan's point was that both code versions shouldn't be any different. >>> >>> # option-1 >>> if (a) >>> return f1(); >>> else >>> return f2(); >>> >>> # option-2 >>> if (a) >>> return f1(); >>> return f2(); >>> >>> Both options are equally guaranteed to always return. It looks like a >>> peculiar limitation to only recognize option-2 as something that >>> returns >>> 100% of the times. Also option-1 returns 100% of the times. What am I >>> missing? > > I don't think this is necessarily a limitation because it highlights > cases where an if-else could be replaced with a simple if: > some may find an if-else clearer, other may find the single if clearer. > > From a safety point of view both options are safe because there > is no risk of unintentional fall through. > > If you all agree on the fact that small code refactoring like the ones I > proposed are counterproductive, then I can update the tool configuration > to consider also option-1 as safe. I would certainly prefer if we could continue to use option 1. ~Andrew
Re: [XEN PATCH 0/7] xen/arm: address violations of MISRA C:2012 Rule 16.3
On 21/12/2023 8:08 am, Jan Beulich wrote:
> On 20.12.2023 22:35, Andrew Cooper wrote:
>> On 20/12/2023 11:03 am, Federico Serafini wrote:
>>> This patch series addresses violations of MISRA C:2012 Rule 16.3 on the Arm
>>> code. No fucntional changes are introduced.
>>>
>>> Federico Serafini (7):
>>> xen/arm: gic-v3: address violations of MISRA C:2012 Rule 16.3
>>> xen/arm: traps: address violations of MISRA C:2012 Rule 16.3
>>> xen/arm: guest_walk: address violations of MISRA C:2012 Rule 16.3
>>> xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
>>> xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
>>> xen/arm: mmu: address a violations of MISRA C:2012 Rule 16.3
>>> xen/arm: smmu-v3: address violations of MISRA C:2012 Rule 16.3
>>>
>>> xen/arch/arm/arm64/vsysreg.c | 4 ++--
>>> xen/arch/arm/gic-v3.c | 30 +++
>>> xen/arch/arm/guest_walk.c | 4
>>> xen/arch/arm/mem_access.c | 12 +--
>>> xen/arch/arm/mmu/p2m.c| 1 +
>>> xen/arch/arm/traps.c | 18
>>> xen/arch/arm/vcpreg.c | 4 ++--
>>> xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
>>> 8 files changed, 61 insertions(+), 14 deletions(-)
>>>
>> Just a couple of notes on style. This isn't a request to change
>> anything in this series, particularly as most is already committed, but
>> bear it in mind for what I expect will be similar patches in other areas.
>>
>> We explicitly permit tabulation when it aids readibility, so patch 2
>> could have been written:
>>
>> switch ( hypercall_args[*nr] ) {
>> case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEFU; fallthrough;
>> case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEFU; fallthrough;
>> case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEFU; fallthrough;
>> case 2: HYPERCALL_ARG2(regs) = 0xDEADBEEFU; fallthrough;
>> case 1: /* Don't clobber x0/r0 -- it's the return value */
>> case 0: /* -ENOSYS case */
>> break;
>> default: BUG();
>> }
>>
>> (give or take the brace placement other style issue) We also have cases
>> where a break before a new case statement is preferred, i.e.:
> Did you mean "blank line" here, seeing ...
>
>> ...
>> break;
>>
>> case ...:
>>
>> This is to prevent larger switch statements from being a straight wall
>> of text.
> ... this as the further explanation?
Urgh yes - I did mean blank line. Hopefully the intent was clear.
~Andrew
[XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
Remove declarations of __put_user_bad() and __get_user_bad()
since they have no definition.
Replace their uses with a break statement to address violations of
MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
terminate every switch-clause").
No functional change.
Signed-off-by: Federico Serafini
---
Several violations of Rule 16.3 come from uses of macros
get_unsafe_size() and put_unsafe_size().
Looking at the macro definitions I found __get_user_bad() and __put_user_bad().
I was wondering if instead of just adding the break statement I can also remove
such functions which seem to not have a definition.
---
xen/arch/x86/include/asm/uaccess.h | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/include/asm/uaccess.h
b/xen/arch/x86/include/asm/uaccess.h
index 7443519d5b..15b747d0c7 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -21,9 +21,6 @@ unsigned int copy_from_guest_ll(void *to, const void __user
*from, unsigned int
unsigned int copy_to_unsafe_ll(void *to, const void *from, unsigned int n);
unsigned int copy_from_unsafe_ll(void *to, const void *from, unsigned int n);
-extern long __get_user_bad(void);
-extern void __put_user_bad(void);
-
#define UA_KEEP(args...) args
#define UA_DROP(args...)
@@ -208,7 +205,7 @@ do {
\
case 8:\
put_unsafe_asm(x, ptr, grd, retval, "q", "", "ir", errret); \
break; \
-default: __put_user_bad(); \
+default: break;\
} \
clac();\
} while ( false )
@@ -227,7 +224,7 @@ do {
\
case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); break; \
case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); break; \
case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
-default: __get_user_bad(); \
+default: break;\
} \
clac();\
} while ( false )
--
2.34.1
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 21.12.2023 11:53, Federico Serafini wrote:
> Remove declarations of __put_user_bad() and __get_user_bad()
> since they have no definition.
> Replace their uses with a break statement to address violations of
> MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
> terminate every switch-clause").
> No functional change.
>
> Signed-off-by: Federico Serafini
> ---
> Several violations of Rule 16.3 come from uses of macros
> get_unsafe_size() and put_unsafe_size().
> Looking at the macro definitions I found __get_user_bad() and
> __put_user_bad().
> I was wondering if instead of just adding the break statement I can also
> remove
> such functions which seem to not have a definition.
No, you can't. Try introducing a caller which "accidentally" uses the
wrong size. Without your change you'll observe the build failing (in
a somewhat obscure way, but still), while with your change bad code
will silently be generated.
Jan
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 21/12/2023 10:58 am, Jan Beulich wrote:
> On 21.12.2023 11:53, Federico Serafini wrote:
>> Remove declarations of __put_user_bad() and __get_user_bad()
>> since they have no definition.
>> Replace their uses with a break statement to address violations of
>> MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
>> terminate every switch-clause").
>> No functional change.
>>
>> Signed-off-by: Federico Serafini
>> ---
>> Several violations of Rule 16.3 come from uses of macros
>> get_unsafe_size() and put_unsafe_size().
>> Looking at the macro definitions I found __get_user_bad() and
>> __put_user_bad().
>> I was wondering if instead of just adding the break statement I can also
>> remove
>> such functions which seem to not have a definition.
> No, you can't. Try introducing a caller which "accidentally" uses the
> wrong size. Without your change you'll observe the build failing (in
> a somewhat obscure way, but still), while with your change bad code
> will silently be generated.
The construct here is deliberate. It's a build time assertion that bad
sizes aren't used.
__bitop_bad_size() and __xsm_action_mismatch_detected() are the same
pattern in other areas of code too, with the latter being more explicit
because of how it's wrapped by LINKER_BUG_ON().
It is slightly horrible, and not the most obvious construct for
newcomers. If there's an alternative way to get a build assertion, we
could consider switching to a new pattern.
~Andrew
Re: [PATCH v4 03/12] xen/spinlock: introduce new type for recursive spinlocks
On 21.12.23 11:34, Jan Beulich wrote:
On 12.12.2023 13:57, Julien Grall wrote:
On 12/12/2023 09:47, Juergen Gross wrote:
@@ -109,12 +109,16 @@ struct lock_profile_qhead {
spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);
\
static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);
\
_LOCK_PROFILE_PTR(l)
+#define DEFINE_RSPINLOCK(l) \
+rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);\
+static struct lock_profile __lock_profile_data_##l = _LOCK_PROFILE(l);\
+_LOCK_PROFILE_PTR(l)
-#define spin_lock_init_prof(s, l) \
+#define __spin_lock_init_prof(s, l, locktype) \
If I am not mistaken the double-underscore prefix is a violation in
MISRA. So can this be renamed to:
spin_lock_init_prof__()?
Is the new parameter needed at all? Can't we use typeof((s)->l) in place of
passing the type explicitly?
IMO spin_lock_init_prof() should be usable for spinlock_t only, and
rspin_lock_init_prof() for rspinlock_t only. Using typeof() would make
either of them usable for both types.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 03/12] xen/spinlock: introduce new type for recursive spinlocks
On 21.12.2023 12:06, Juergen Gross wrote:
> On 21.12.23 11:34, Jan Beulich wrote:
>> On 12.12.2023 13:57, Julien Grall wrote:
>>> On 12/12/2023 09:47, Juergen Gross wrote:
@@ -109,12 +109,16 @@ struct lock_profile_qhead {
spinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);
\
static struct lock_profile __lock_profile_data_##l =
_LOCK_PROFILE(l);\
_LOCK_PROFILE_PTR(l)
+#define DEFINE_RSPINLOCK(l)
\
+rspinlock_t l = _SPIN_LOCK_UNLOCKED(NULL);
\
+static struct lock_profile __lock_profile_data_##l =
_LOCK_PROFILE(l);\
+_LOCK_PROFILE_PTR(l)
-#define spin_lock_init_prof(s, l)
\
+#define __spin_lock_init_prof(s, l, locktype)
\
>>>
>>> If I am not mistaken the double-underscore prefix is a violation in
>>> MISRA. So can this be renamed to:
>>>
>>> spin_lock_init_prof__()?
>>
>> Is the new parameter needed at all? Can't we use typeof((s)->l) in place of
>> passing the type explicitly?
>
> IMO spin_lock_init_prof() should be usable for spinlock_t only, and
> rspin_lock_init_prof() for rspinlock_t only. Using typeof() would make
> either of them usable for both types.
Fair point.
Jan
Re: [PATCH] x86/iommu: add CHANGELOG entry for hwdom setup time improvements
On Thu, Dec 21, 2023 at 11:23:49AM +0100, Jan Beulich wrote: > On 21.12.2023 11:12, Roger Pau Monne wrote: > > Signed-off-by: Roger Pau Monné > > --- > > CHANGELOG.md | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/CHANGELOG.md b/CHANGELOG.md > > index 5ee5d41fc933..52484c047bd1 100644 > > --- a/CHANGELOG.md > > +++ b/CHANGELOG.md > > @@ -9,6 +9,8 @@ The format is based on [Keep a > > Changelog](https://keepachangelog.com/en/1.0.0/) > > ### Changed > > - Changed flexible array definitions in public I/O interface headers to > > not > > use "1" as the number of array elements. > > + - On x86: > > + - Reduce IOMMU setup time for hardware domain. > > > > ### Added > > - On x86: > > I'm a little puzzled: Isn't this more like patch 7/6 of the v4 series > (or possibly patch 5.5/6), which hasn't gone in yet? Yes, that's why I've sent it as a reply-to the cover letter. I assumed it was clear enough that it's only supposed to go in after the rest of the series. Should have been 7/7, but I forgot about it. Regards, Roger.
Re: [PATCH v2] x86/amd: extend CPU errata #1474 affected models
On Thu, Dec 21, 2023 at 10:24:55AM +, Andrew Cooper wrote: > On 21/12/2023 10:20 am, Jan Beulich wrote: > > On 21.12.2023 11:17, Andrew Cooper wrote: > >> On 21/12/2023 10:08 am, Roger Pau Monne wrote: > >>> Errata #1474has now been extended to cover models from family 17h ranges > >> Extra space needed. Can be fixed on commit. > > Also (not just here) - isn't it "erratum" when we talk of just one? > > Good point, yes. > > I'll fix both. (I'm just collecting some patches for a Gitlab CI run on > Juergen's patches, and I'll include this one.) Thanks, didn't know errata was plural and erratum was singular. Will try to remember. Roger.
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
Hi Andrew,
On 2023-12-21 12:03, Andrew Cooper wrote:
On 21/12/2023 10:58 am, Jan Beulich wrote:
On 21.12.2023 11:53, Federico Serafini wrote:
Remove declarations of __put_user_bad() and __get_user_bad()
since they have no definition.
Replace their uses with a break statement to address violations of
MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
terminate every switch-clause").
No functional change.
Signed-off-by: Federico Serafini
---
Several violations of Rule 16.3 come from uses of macros
get_unsafe_size() and put_unsafe_size().
Looking at the macro definitions I found __get_user_bad() and
__put_user_bad().
I was wondering if instead of just adding the break statement I can
also remove
such functions which seem to not have a definition.
No, you can't. Try introducing a caller which "accidentally" uses the
wrong size. Without your change you'll observe the build failing (in
a somewhat obscure way, but still), while with your change bad code
will silently be generated.
The construct here is deliberate. It's a build time assertion that bad
sizes aren't used.
__bitop_bad_size() and __xsm_action_mismatch_detected() are the same
pattern in other areas of code too, with the latter being more explicit
because of how it's wrapped by LINKER_BUG_ON().
It is slightly horrible, and not the most obvious construct for
newcomers. If there's an alternative way to get a build assertion, we
could consider switching to a new pattern.
~Andrew
would you be in favour of a solution with a BUILD_BUG_ON in the default
branch followed by a break?
default:
BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
break;
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
On 20.12.2023 17:34, Sébastien Chaumat wrote:
> Here are the patches I made to xen and linux kernel
> Plus dmesg (bare metal,xen) and "xl dmesg"
So the problem looks to be that pci_xen_initial_domain() results in
permanent setup of IRQ7, when there only "static" ACPI tables (in
particular source overrides in MADT) are consulted. The necessary
settings, however, are known only after _CRS for the device was
evaluated (and possibly _PRS followed by invocation of _SRS). All of
this happens before xen_register_gsi() is called. But that function's
call to xen_register_pirq() is short-circuited by the very first if()
in xen_register_pirq() when there was an earlier invocation. Hence
the (wrong) "edge" binding remains in place, as was established by
the earlier call here.
Jürgen, there's an interesting comment in xen_bind_pirq_gsi_to_irq(),
right before invoking irq_set_chip_and_handler_name(). Despite what
the comment says (according to my reading), the fasteoi one is _not_
used in all cases. Assuming there's a reason for this, it's not clear
to me whether updating the handler later on is a valid thing to do.
__irq_set_handler() being even an exported symbol suggests that might
be an option to use here. Then again merely updating the handler may
not be sufficient, seeing there are also e.g. IRQD_TRIGGER_MASK and
IRQD_LEVEL.
Sébastien, to prove the (still pretty weak) theory that the change in
handler is all that's needed to make things work in your case, could
you fiddle with pci_xen_initial_domain() to have it skip IRQ7? (That
of course won't be a proper solution, but ought to be okay for your
system.) The main weakness of the theory is that IRQ7 really isn't
very special in this regard - other PCI IRQs routed to the low 16
IO-APIC pins ought to have similar issues (from the log, on your
system this would be at least IRQ6 and IRQ10, except that they happen
to be edge/low, so fitting the ISA defaults); only IRQ16 and up would
work okay.
Furthermore it might be interesting to know whether ELCR would give us
any hint in this case. Sadly depending on where you look,
applicability of this pair of registers (I/O ports 0x4d0 and 0x4d1)
to other than EISA systems is claimed true or false. Could you perhaps
make Xen simply log the values read from those two ports, by e.g.
inserting
printk("ELCR: %02x, %02x\n", inb(0x4d0), inb(0x4d1));
in, say, setup_dump_irqs()?
Jürgen, looking at pci_xen_initial_domain(), what's the purpose of
the loop in the final if()? Can this ever do anything useful when
the earlier comment suggests nr_legacy_irqs() is zero anyway? Or is
the comment simply inaccurate in not covering the "no IO-APICs" case?
Jan
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 21.12.2023 13:01, Nicola Vetrini wrote:
> Hi Andrew,
>
> On 2023-12-21 12:03, Andrew Cooper wrote:
>> On 21/12/2023 10:58 am, Jan Beulich wrote:
>>> On 21.12.2023 11:53, Federico Serafini wrote:
Remove declarations of __put_user_bad() and __get_user_bad()
since they have no definition.
Replace their uses with a break statement to address violations of
MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
terminate every switch-clause").
No functional change.
Signed-off-by: Federico Serafini
---
Several violations of Rule 16.3 come from uses of macros
get_unsafe_size() and put_unsafe_size().
Looking at the macro definitions I found __get_user_bad() and
__put_user_bad().
I was wondering if instead of just adding the break statement I can
also remove
such functions which seem to not have a definition.
>>> No, you can't. Try introducing a caller which "accidentally" uses the
>>> wrong size. Without your change you'll observe the build failing (in
>>> a somewhat obscure way, but still), while with your change bad code
>>> will silently be generated.
>>
>> The construct here is deliberate. It's a build time assertion that bad
>> sizes aren't used.
>>
>> __bitop_bad_size() and __xsm_action_mismatch_detected() are the same
>> pattern in other areas of code too, with the latter being more explicit
>> because of how it's wrapped by LINKER_BUG_ON().
>>
>>
>> It is slightly horrible, and not the most obvious construct for
>> newcomers. If there's an alternative way to get a build assertion, we
>> could consider switching to a new pattern.
>
> would you be in favour of a solution with a BUILD_BUG_ON in the default
> branch followed by a break?
>
> default:
> BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
> break;
I don't think this would compile - BUILD_BUG_ON() wants a compile-time
constant passed.
Jan
[ovmf test] 184202: all pass - PUSHED
flight 184202 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184202/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 5d533bbc27732a421e3bf35c5af77782b8a85e6f baseline version: ovmf 9f0061a03b61d282fbc0ba5be22155d06a5e64a1 Last test of basis 184198 2023-12-21 01:57:56 Z0 days Testing same since 184202 2023-12-21 10:12:56 Z0 days1 attempts People who touched revisions under test: Jake Garver 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 9f0061a03b..5d533bbc27 5d533bbc27732a421e3bf35c5af77782b8a85e6f -> xen-tested-master
Governance change proposal on small updates
Hi all, I am proposing a small change in how we update non-trivial matters in our governance. Currently, any governance change requires a formal vote. However, there will be times when this is unnecessary and would hinder progress in the project. For example, my previous email proposal on changes to clarity and inclusivity language. As it stands, I have not received any pushback or feedback from the community. To help us progress faster, I would suggest the following: -* Small changes will still need to be proposed to xen-devel and the community* *- The community is welcome to give as much feedback as necessary before any changes are made* *- Proposals can be changed/updated as needed, then resubmitted to the community * *- Anyone can object to these changes or call a vote within 30 days of the proposal if deemed necessary* *- A committer must ack the change for it to go ahead* *- If the community manager does not hear any feedback within 30 days, the changes can be acked by a committer and put into the governance* *- All important matters and policy changes to the community will still go through a formal voting process. This change only applies to small matters within the governance. * Examples: *- Wording or spelling changes* *- Updating small sentences or clarity changes* *- Adding examples to existing code of conduct policies* I welcome your thoughts on the above proposal. Please reply by 14th January 2024 should you have any objections to this. If by lazy consensus I do not hear back from this date, I will assume I have your agreement on this. Many thanks, Kelly Choi Community Manager Xen Project On Fri, Nov 24, 2023 at 10:57 AM Kelly Choi wrote: > Hi all, > > Please see an updated Governance PR on GitLab here: > https://gitlab.com/xen-project/governance/governance/-/merge_requests/1 > > Comments: > > Revise code of conduct for enhanced clarity, inclusivity, and > accountability > > In response to valuable feedback from community members and in alignment > with our ongoing commitment to creating a safe and welcoming space for > collaboration, this commit refines the code of conduct. The changes focus > on: > >- *Clarity:* Rewording sections to eliminate ambiguity and ensure that >expectations are clearly communicated. >- *Inclusivity:* Adding language to emphasize our dedication to >diversity and inclusion, and providing examples to illustrate the types of >behavior we encourage. > > These updates aim to foster a more positive and collaborative atmosphere > within our community. Please review the changes and don't hesitate to > provide further input or suggestions. > > Note that the patches should be read as a whole; I'm still learning git > and using the gitlab UI, which doesn't have a way to do history editing. > Many thanks, > Kelly Choi > > Open Source Community Manager > XenServer, Cloud Software Group >
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
On 21.12.23 13:40, Jan Beulich wrote:
On 20.12.2023 17:34, Sébastien Chaumat wrote:
Here are the patches I made to xen and linux kernel
Plus dmesg (bare metal,xen) and "xl dmesg"
So the problem looks to be that pci_xen_initial_domain() results in
permanent setup of IRQ7, when there only "static" ACPI tables (in
particular source overrides in MADT) are consulted. The necessary
settings, however, are known only after _CRS for the device was
evaluated (and possibly _PRS followed by invocation of _SRS). All of
this happens before xen_register_gsi() is called. But that function's
call to xen_register_pirq() is short-circuited by the very first if()
in xen_register_pirq() when there was an earlier invocation. Hence
the (wrong) "edge" binding remains in place, as was established by
the earlier call here.
Jürgen, there's an interesting comment in xen_bind_pirq_gsi_to_irq(),
right before invoking irq_set_chip_and_handler_name(). Despite what
the comment says (according to my reading), the fasteoi one is _not_
used in all cases. Assuming there's a reason for this, it's not clear
to me whether updating the handler later on is a valid thing to do.
__irq_set_handler() being even an exported symbol suggests that might
be an option to use here. Then again merely updating the handler may
not be sufficient, seeing there are also e.g. IRQD_TRIGGER_MASK and
IRQD_LEVEL.
I understand the last paragraph of that comment to reason, that in case
pirq_needs_eoi() return true even in case of an edge triggered interrupt,
the outcome is still okay.
I don't think updating the handler later is valid.
Sébastien, to prove the (still pretty weak) theory that the change in
handler is all that's needed to make things work in your case, could
you fiddle with pci_xen_initial_domain() to have it skip IRQ7? (That
of course won't be a proper solution, but ought to be okay for your
system.) The main weakness of the theory is that IRQ7 really isn't
very special in this regard - other PCI IRQs routed to the low 16
IO-APIC pins ought to have similar issues (from the log, on your
system this would be at least IRQ6 and IRQ10, except that they happen
to be edge/low, so fitting the ISA defaults); only IRQ16 and up would
work okay.
Furthermore it might be interesting to know whether ELCR would give us
any hint in this case. Sadly depending on where you look,
applicability of this pair of registers (I/O ports 0x4d0 and 0x4d1)
to other than EISA systems is claimed true or false. Could you perhaps
make Xen simply log the values read from those two ports, by e.g.
inserting
printk("ELCR: %02x, %02x\n", inb(0x4d0), inb(0x4d1));
in, say, setup_dump_irqs()?
Jürgen, looking at pci_xen_initial_domain(), what's the purpose of
the loop in the final if()? Can this ever do anything useful when
the earlier comment suggests nr_legacy_irqs() is zero anyway? Or is
the comment simply inaccurate in not covering the "no IO-APICs" case?
No, I think the final loop would only do something in case probe_8259A()
is detecting a working PIC (which should not be the case IMHO). Could it
be that there have been Xen versions emulating a PIC?
The call of probe_8259A() is in no way depending on nr_ioapics.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
xen | Successful pipeline for staging | 3909fb46
Pipeline #1115493558 has passed! Project: xen ( https://gitlab.com/xen-project/xen ) Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging ) Commit: 3909fb46 ( https://gitlab.com/xen-project/xen/-/commit/3909fb4692dfbf7e46c0bcc37b0a3b943a034da9 ) Commit Message: docs/misra: exclude files inherited from ACPI C... Commit Author: Nicola Vetrini Committed by: Jan Beulich ( https://gitlab.com/jbeulich ) Pipeline #1115493558 ( https://gitlab.com/xen-project/xen/-/pipelines/1115493558 ) triggered by Ganis ( https://gitlab.com/ganis ) successfully completed 129 jobs in 3 stages. -- You're receiving this email because of your account on gitlab.com.
[xen-unstable-smoke test] 184203: tolerable all pass - PUSHED
flight 184203 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/184203/ 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 3909fb4692dfbf7e46c0bcc37b0a3b943a034da9 baseline version: xen 6efc654a0b529a0e4d08e5d6bb5762fee1bf1eac Last test of basis 184195 2023-12-20 22:00:27 Z0 days Testing same since 184203 2023-12-21 11:00:27 Z0 days1 attempts People who touched revisions under test: Jan Beulich Nicola Vetrini Roger Pau Monné Stefano Stabellini Stewart Hildebrand Volodymyr Babchuk 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 6efc654a0b..3909fb4692 3909fb4692dfbf7e46c0bcc37b0a3b943a034da9 -> smoke
Re: [PATCH v11 03/17] vpci: use per-domain PCI lock to protect vpci structure
On Sat, Dec 02, 2023 at 01:27:03AM +, Volodymyr Babchuk wrote: > From: Oleksandr Andrushchenko > > Use a previously introduced per-domain read/write lock to check > whether vpci is present, so we are sure there are no accesses to the > contents of the vpci struct if not. This lock can be used (and in a > few cases is used right away) so that vpci removal can be performed > while holding the lock in write mode. Previously such removal could > race with vpci_read for example. > > When taking both d->pci_lock and pdev->vpci->lock, they should be > taken in this exact order: d->pci_lock then pdev->vpci->lock to avoid > possible deadlock situations. > > 1. Per-domain's pci_rwlock is used to protect pdev->vpci structure > from being removed. > > 2. Writing the command register and ROM BAR register may trigger > modify_bars to run, which in turn may access multiple pdevs while > checking for the existing BAR's overlap. The overlapping check, if > done under the read lock, requires vpci->lock to be acquired on both > devices being compared, which may produce a deadlock. It is not > possible to upgrade read lock to write lock in such a case. So, in > order to prevent the deadlock, use d->pci_lock instead. To prevent > deadlock while locking both hwdom->pci_lock and dom_xen->pci_lock, > always lock hwdom first. FWIW (I think it was also mentioned in the previous version) for devices assigned to dom_xen taking the hwdom lock should be enough. There are no other accesses to such devices that don't originate from hwdom, and it's not possible to degassing devices from dom_xen. > > All other code, which doesn't lead to pdev->vpci destruction and does > not access multiple pdevs at the same time, can still use a > combination of the read lock and pdev->vpci->lock. > > 3. Drop const qualifier where the new rwlock is used and this is > appropriate. > > 4. Do not call process_pending_softirqs with any locks held. For that > unlock prior the call and re-acquire the locks after. After > re-acquiring the lock there is no need to check if pdev->vpci exists: > - in apply_map because of the context it is called (no race condition >possible) > - for MSI/MSI-X debug code because it is called at the end of >pdev->vpci access and no further access to pdev->vpci is made > > 5. Use d->pci_lock around for_each_pdev and pci_get_pdev_by_domain > while accessing pdevs in vpci code. > > 6. We are removing multiple ASSERT(pcidevs_locked()) instances because > they are too strict now: they should be corrected to > ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)), but problem is > that mentioned instances does not have access to the domain > pointer and it is not feasible to pass a domain pointer to a function > just for debugging purposes. > > Suggested-by: Roger Pau Monné > Suggested-by: Jan Beulich > Signed-off-by: Oleksandr Andrushchenko > Signed-off-by: Volodymyr Babchuk > > --- > Changes in v11: > - Fixed commit message regarding possible spinlocks > - Removed parameter from allocate_and_map_msi_pirq(), which was added > in the prev version. Now we are taking pcidevs_lock in > physdev_map_pirq() > - Returned ASSERT to pci_enable_msi > - Fixed case when we took read lock instead of write one > - Fixed label indentation > > Changes in v10: > - Moved printk pas locked area > - Returned back ASSERTs > - Added new parameter to allocate_and_map_msi_pirq() so it knows if > it should take the global pci lock > - Added comment about possible improvement in vpci_write > - Changed ASSERT(rw_is_locked()) to rw_is_write_locked() in >appropriate places > - Renamed release_domain_locks() to release_domain_write_locks() > - moved domain_done label in vpci_dump_msi() to correct place > Changes in v9: > - extended locked region to protect vpci_remove_device and >vpci_add_handlers() calls > - vpci_write() takes lock in the write mode to protect >potential call to modify_bars() > - renamed lock releasing function > - removed ASSERT()s from msi code > - added trylock in vpci_dump_msi > > Changes in v8: > - changed d->vpci_lock to d->pci_lock > - introducing d->pci_lock in a separate patch > - extended locked region in vpci_process_pending > - removed pcidevs_lockis vpci_dump_msi() > - removed some changes as they are not needed with >the new locking scheme > - added handling for hwdom && dom_xen case > --- > xen/arch/x86/hvm/vmsi.c | 22 +++ > xen/arch/x86/hvm/vmx/vmx.c| 2 -- > xen/arch/x86/irq.c| 8 +++--- > xen/arch/x86/msi.c| 10 ++- > xen/arch/x86/physdev.c| 2 ++ > xen/drivers/passthrough/pci.c | 9 +++--- > xen/drivers/vpci/header.c | 18 > xen/drivers/vpci/msi.c| 28 +-- > xen/drivers/vpci/msix.c | 52 ++- > xen/drivers/vpci/vpci.c | 50 +++-- > 10 files changed, 160 insertions(+), 41 deletions(-)
[xen-unstable test] 184197: tolerable FAIL - PUSHED
flight 184197 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/184197/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184189 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184189 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184189 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184189 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184189 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184189 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184189 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184189 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184189 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184189 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184189 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184189 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail 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-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 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-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-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-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail 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-amd64-amd64-libvirt-vhd 14 migrate-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-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass version targeted for testing: xen e9786ba9ee5f0b4b6eadb4ca80417f195ce251a0 baseline version: xen 725acf8e4d406bf0a24441ac1738eb6f4f6ef0c3 Last test of basis 184189 2023-12-20 09:47:47 Z1 days Testing same since 184197 2023-12-20 23:39:14 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Federico Serafini Jan Beulich
Re: [PATCH RFC v3 for-6.8/block 12/17] nilfs2: use bdev api in nilfs_attach_log_writer()
On Thu, Dec 21, 2023 at 6:00 PM Yu Kuai wrote: > > From: Yu Kuai > > Avoid to access bd_inode directly, prepare to remove bd_inode from > block_device. > > Signed-off-by: Yu Kuai > --- > fs/nilfs2/segment.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c > index 55e31cc903d1..a1130e384937 100644 > --- a/fs/nilfs2/segment.c > +++ b/fs/nilfs2/segment.c > @@ -2823,7 +2823,7 @@ int nilfs_attach_log_writer(struct super_block *sb, > struct nilfs_root *root) > if (!nilfs->ns_writer) > return -ENOMEM; > > - inode_attach_wb(nilfs->ns_bdev->bd_inode, NULL); > + bdev_attach_wb(nilfs->ns_bdev); > > err = nilfs_segctor_start_thread(nilfs->ns_writer); > if (unlikely(err)) > -- > 2.39.2 > Acked-by: Ryusuke Konishi Thanks, Ryusuke Konishi
Re: [PATCH v2 17/39] xen/riscv: introduce asm/atomic.h
On Thu, 2023-12-07 at 16:57 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > From: Bobby Eshleman
> >
> > Signed-off-by: Oleksii Kurochko
> > ---
> > Changes in V2:
> > - Change an author of commit. I got this header from Bobby's old
> > repo.
>
> Not sure how to deal with that when there's not also an S-o-b.
>
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -0,0 +1,375 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Taken and modified from Linux.
> > + *
> > + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2017 SiFive
> > + * Copyright (C) 2021 Vates SAS
> > + */
> > +
> > +#ifndef _ASM_RISCV_ATOMIC_H
> > +#define _ASM_RISCV_ATOMIC_H
> > +
> > +#include
> > +#include
>
> This and ...
>
> > +#include
> > +#include
>
> .. this header are only introduced later. Bad ordering of the series?
Yes, bad ordering. I'll re-order this patch
Thanks.
>
> > +#include
> > +
> > +void __bad_atomic_size(void);
> > +
> > +static always_inline void read_atomic_size(const volatile void *p,
> > + void *res,
> > + unsigned int size)
> > +{
> > + switch ( size ) {
>
> Nit (style): Brace on its own line (again further down).
>
> > + case 1: *(uint8_t *)res = readb((uint8_t *)p); break;
> > + case 2: *(uint16_t *)res = readw((uint16_t *)p); break;
> > + case 4: *(uint32_t *)res = readl((uint32_t *)p); break;
> > + case 8: *(uint32_t *)res = readq((uint64_t *)p); break;
>
> Please don't cast away const-ness.
>
> > + default: __bad_atomic_size(); break;
> > + }
> > +}
> > +
> > +#define read_atomic(p)
> > ({ \
> > + union { typeof(*p) val; char c[0]; }
> > x_; \
>
> Hmm, you avoid leading underscores here, but then ...
>
> > + read_atomic_size(p, x_.c,
> > sizeof(*p)); \
> > +
> > x_.val;
> > \
> > +})
> > +
> > +
> > +#define write_atomic(p, x)
> > ({ \
> > + typeof(*p) __x =
> > (x); \
>
> ... they're still there here.
I'll rename __x to x__. Thanks.
>
> > + switch ( sizeof(*p) )
> > { \
> > + case 1: writeb((uint8_t)__x, (uint8_t *) p);
> > break; \
> > + case 2: writew((uint16_t)__x, (uint16_t *) p);
> > break; \
> > + case 4: writel((uint32_t)__x, (uint32_t *) p);
> > break; \
> > + case 8: writeq((uint64_t)__x, (uint64_t *) p);
> > break; \
> > + default: __bad_atomic_size();
> > break; \
> > +
> > }
> > \
> > +
> > __x;
> > \
> > +})
> > +
> > +/* TODO: Fix this */
> > +#define add_sized(p, x)
> > ({ \
> > + typeof(*(p)) __x =
> > (x); \
> > + switch ( sizeof(*(p))
> > ) \
> > +
> > {
> > \
> > + case 1: writeb(read_atomic(p) + __x, (uint8_t *)(p));
> > break; \
> > + case 2: writew(read_atomic(p) + __x, (uint16_t *)(p));
> > break; \
> > + case 4: writel(read_atomic(p) + __x, (uint32_t *)(p));
> > break; \
>
> Instead of this, considering the comment perhaps better just BUG()?
I think this TODO can be removed. Macros looks fine to me.
>
> > + default: __bad_atomic_size();
> > break; \
> > +
> > }
> > \
> > +})
> > +
> > +/*
> > + * __unqual_scalar_typeof(x) - Declare an unqualified scalar
> > type, leaving
> > + * non-scalar types unchanged.
> > + *
> > + * Prefer C11 _Generic for better compile-times and simpler code.
> > Note: 'char'
> > + * is not type-compatible with 'signed char', and we define a
> > separate case.
> > + */
> > +#define __scalar_type_to_expr_cases(type) \
> > + unsigned type: (unsigned type)0, \
> > + signed type: (signed type)0
> > +
> > +#define __unqual_scalar_typeof(x) typeof( \
> > + _Generic((x), \
>
> I think you still owe us an update to ./README, clarifying what
> compiler versions
> may be used for building RISC-V. Unless of course all that exist
> support _Generic
> (which then would be nice to say in the description).
I'll provide a separate patch.
>
> > + char: (char)0, \
Re: [PATCH v11 05/17] vpci: add hooks for PCI device assign/de-assign
On Sat, Dec 02, 2023 at 01:27:03AM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko
>
> When a PCI device gets assigned/de-assigned we need to
> initialize/de-initialize vPCI state for the device.
>
> Also, rename vpci_add_handlers() to vpci_assign_device() and
> vpci_remove_device() to vpci_deassign_device() to better reflect role
> of the functions.
>
> Signed-off-by: Oleksandr Andrushchenko
> Signed-off-by: Volodymyr Babchuk
Reviewed-by: Roger Pau Monné
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index d743d96a10..75cfb532ee 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -25,11 +25,11 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>static vpci_register_init_t *const x##_entry \
> __used_section(".data.vpci." p) = x
>
> -/* Add vPCI handlers to device. */
> -int __must_check vpci_add_handlers(struct pci_dev *pdev);
> +/* Assign vPCI to device by adding handlers to device. */
Nit: the comment would likely benefit from removing the last device
before the full stop.
> +int __must_check vpci_assign_device(struct pci_dev *pdev);
Thanks, Roger.
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
On 21.12.2023 14:29, Juergen Gross wrote: > On 21.12.23 13:40, Jan Beulich wrote: >> On 20.12.2023 17:34, Sébastien Chaumat wrote: >>> Here are the patches I made to xen and linux kernel >>> Plus dmesg (bare metal,xen) and "xl dmesg" >> >> So the problem looks to be that pci_xen_initial_domain() results in >> permanent setup of IRQ7, when there only "static" ACPI tables (in >> particular source overrides in MADT) are consulted. The necessary >> settings, however, are known only after _CRS for the device was >> evaluated (and possibly _PRS followed by invocation of _SRS). All of >> this happens before xen_register_gsi() is called. But that function's >> call to xen_register_pirq() is short-circuited by the very first if() >> in xen_register_pirq() when there was an earlier invocation. Hence >> the (wrong) "edge" binding remains in place, as was established by >> the earlier call here. >> >> Jürgen, there's an interesting comment in xen_bind_pirq_gsi_to_irq(), >> right before invoking irq_set_chip_and_handler_name(). Despite what >> the comment says (according to my reading), the fasteoi one is _not_ >> used in all cases. Assuming there's a reason for this, it's not clear >> to me whether updating the handler later on is a valid thing to do. >> __irq_set_handler() being even an exported symbol suggests that might >> be an option to use here. Then again merely updating the handler may >> not be sufficient, seeing there are also e.g. IRQD_TRIGGER_MASK and >> IRQD_LEVEL. > > I understand the last paragraph of that comment to reason, that in case > pirq_needs_eoi() return true even in case of an edge triggered interrupt, > the outcome is still okay. > > I don't think updating the handler later is valid. In which case fixing this is going to be interesting. >> Jürgen, looking at pci_xen_initial_domain(), what's the purpose of >> the loop in the final if()? Can this ever do anything useful when >> the earlier comment suggests nr_legacy_irqs() is zero anyway? Or is >> the comment simply inaccurate in not covering the "no IO-APICs" case? > > No, I think the final loop would only do something in case probe_8259A() > is detecting a working PIC (which should not be the case IMHO). Could it > be that there have been Xen versions emulating a PIC? Not as far back as I can recall (for PV Dom0). Even in 3.2 Dom0 was already denied access to the respective I/O ports. And iirc upstream Linux wants at least 4.0? Jan
Re: [PATCH v2 29/39] xen/riscv: add definition of __read_mostly
On 12/12/2023 5:04 pm, Jan Beulich wrote: > On 24.11.2023 11:30, Oleksii Kurochko wrote: >> The definition of __read_mostly should be removed in: >> https://lore.kernel.org/xen-devel/f25eb5c9-7c14-6e23-8535-2c66772b3...@suse.com/ > Andrew, can we settle on what to do with that patch? If you don't like me > putting __read_mostly in xen/cache.h (consistent with __ro_after_init), > would you please make an alternative suggestion? xen/linkage.h? xen/sections.h? Sorry - I didn't mean to block it specifically, but I do think xen/cache.h is the wrong place for both to live and that it's a small enough change to warrant sorting out nicely once and for all. ~Andrew
Re: [PATCH v11 07/17] vpci/header: implement guest BAR register handlers
On Sat, Dec 02, 2023 at 01:27:04AM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko
>
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
>
> Emulate guest BAR register values: this allows creating a guest view
> of the registers and emulates size and properties probe as it is done
> during PCI device enumeration by the guest.
>
> All empty, IO and ROM BARs for guests are emulated by returning 0 on
> reads and ignoring writes: this BARs are special with this respect as
> their lower bits have special meaning, so returning default ~0 on read
> may confuse guest OS.
>
> Signed-off-by: Oleksandr Andrushchenko
> Signed-off-by: Volodymyr Babchuk
Just a couple of nits.
Reviewed-by: Roger Pau Monné
> ---
> In v11:
> - Access guest_addr after adjusting for MEM64_HI bar in
> guest_bar_write()
> - guest bar handlers renamed and now _mem_ part to denote
> that they are handling only memory BARs
> - refuse to update guest BAR address if BAR is enabled
> In v10:
> - ull -> ULL to be MISRA-compatbile
> - Use PAGE_OFFSET() instead of combining with ~PAGE_MASK
> - Set type of empty bars to VPCI_BAR_EMPTY
> In v9:
> - factored-out "fail" label introduction in init_bars()
> - replaced #ifdef CONFIG_X86 with IS_ENABLED()
> - do not pass bars[i] to empty_bar_read() handler
> - store guest's BAR address instead of guests BAR register view
> Since v6:
> - unify the writing of the PCI_COMMAND register on the
> error path into a label
> - do not introduce bar_ignore_access helper and open code
> - s/guest_bar_ignore_read/empty_bar_read
> - update error message in guest_bar_write
> - only setup empty_bar_read for IO if !x86
> Since v5:
> - make sure that the guest set address has the same page offset
> as the physical address on the host
> - remove guest_rom_{read|write} as those just implement the default
> behaviour of the registers not being handled
> - adjusted comment for struct vpci.addr field
> - add guest handlers for BARs which are not handled and will otherwise
> return ~0 on read and ignore writes. The BARs are special with this
> respect as their lower bits have special meaning, so returning ~0
> doesn't seem to be right
> Since v4:
> - updated commit message
> - s/guest_addr/guest_reg
> Since v3:
> - squashed two patches: dynamic add/remove handlers and guest BAR
> handler implementation
> - fix guest BAR read of the high part of a 64bit BAR (Roger)
> - add error handling to vpci_assign_device
> - s/dom%pd/%pd
> - blank line before return
> Since v2:
> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
> has been eliminated from being built on x86
> Since v1:
> - constify struct pci_dev where possible
> - do not open code is_system_domain()
> - simplify some code3. simplify
> - use gdprintk + error code instead of gprintk
> - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
>so these do not get compiled for x86
> - removed unneeded is_system_domain check
> - re-work guest read/write to be much simpler and do more work on write
>than read which is expected to be called more frequently
> - removed one too obvious comment
> ---
> xen/drivers/vpci/header.c | 135 +-
> xen/include/xen/vpci.h| 3 +
> 2 files changed, 122 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index e6a1d58c42..43216429d9 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -477,6 +477,75 @@ static void cf_check bar_write(
> pci_conf_write32(pdev->sbdf, reg, val);
> }
>
> +static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,
> + unsigned int reg, uint32_t val,
> + void *data)
> +{
> +struct vpci_bar *bar = data;
> +bool hi = false;
> +uint64_t guest_addr;
> +
> +if ( bar->type == VPCI_BAR_MEM64_HI )
> +{
> +ASSERT(reg > PCI_BASE_ADDRESS_0);
> +bar--;
> +hi = true;
> +}
> +else
> +{
> +val &= PCI_BASE_ADDRESS_MEM_MASK;
> +}
> +
> +guest_addr = bar->guest_addr;
> +guest_addr &= ~(0xULL << (hi ? 32 : 0));
> +guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> +
> +/* Allow guest to size BAR correctly */
> +guest_addr &= ~(bar->size - 1);
> +
> +/*
> + * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
> + * writes as long as the BAR is not mapped into the p2m.
> + */
> +if ( bar->enabled )
> +{
> +/* If the value written is the current one avoid printing a warning.
> */
> +if ( guest_addr != bar->guest_addr )
> +gprintk(XENLOG_WARNING,
> +"%pp: ignored guest BAR %zu write while mapped\n",
> +
Re: [PATCH v11 09/17] rangeset: add rangeset_empty() function
On Mon, Dec 04, 2023 at 09:36:16AM +0100, Jan Beulich wrote: > On 02.12.2023 02:27, Volodymyr Babchuk wrote: > > This function can be used when user wants to remove all rangeset > > entries but do not want to destroy rangeset itself. > > I have to admit that I'm not happy with the name: We're not consistently > naming all predicate-like helpers is_...() (see e.g. cpumask_empty()). > May I suggest to use something which unambiguously expresses an action to > be taken, e.g. rangeset_purge(), rangeset_reset(), or (less suitable as > some ambiguity would remain, yet it would be in line with e.g. > cpumask_clear()) rangeset_clear()? rangeset_purge() would be my preference probably, second option would be rangeset_clear(). Thanks, Roger.
Re: [PATCH v11 11/17] vpci/header: program p2m with guest BAR view
On Sat, Dec 02, 2023 at 01:27:05AM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko
>
> Take into account guest's BAR view and program its p2m accordingly:
> gfn is guest's view of the BAR and mfn is the physical BAR value.
> This way hardware domain sees physical BAR values and guest sees
> emulated ones.
>
> Hardware domain continues getting the BARs identity mapped, while for
> domUs the BARs are mapped at the requested guest address without
> modifying the BAR address in the device PCI config space.
>
> Signed-off-by: Oleksandr Andrushchenko
> Signed-off-by: Volodymyr Babchuk
> ---
> In v11:
> - Add vmsix_guest_table_addr() and vmsix_guest_table_base() functions
> to access guest's view of the VMSIx tables.
> - Use MFN (not GFN) to check access permissions
> - Move page offset check to this patch
> - Call rangeset_remove_range() with correct parameters
> In v10:
> - Moved GFN variable definition outside the loop in map_range()
> - Updated printk error message in map_range()
> - Now BAR address is always stored in bar->guest_addr, even for
> HW dom, this removes bunch of ugly is_hwdom() checks in modify_bars()
> - vmsix_table_base() now uses .guest_addr instead of .addr
> In v9:
> - Extended the commit message
> - Use bar->guest_addr in modify_bars
> - Extended printk error message in map_range
> - Moved map_data initialization so .bar can be initialized during declaration
> Since v5:
> - remove debug print in map_range callback
> - remove "identity" from the debug print
> Since v4:
> - moved start_{gfn|mfn} calculation into map_range
> - pass vpci_bar in the map_data instead of start_{gfn|mfn}
> - s/guest_addr/guest_reg
> Since v3:
> - updated comment (Roger)
> - removed gfn_add(map->start_gfn, rc); which is wrong
> - use v->domain instead of v->vpci.pdev->domain
> - removed odd e.g. in comment
> - s/d%d/%pd in altered code
> - use gdprintk for map/unmap logs
> Since v2:
> - improve readability for data.start_gfn and restructure ?: construct
> Since v1:
> - s/MSI/MSI-X in comments
> ---
> xen/drivers/vpci/header.c | 79 +--
> xen/include/xen/vpci.h| 13 +++
> 2 files changed, 73 insertions(+), 19 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 7c84cee5d1..21b3fb5579 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -33,6 +33,7 @@
>
> struct map_data {
> struct domain *d;
> +const struct vpci_bar *bar;
> bool map;
> };
>
> @@ -40,13 +41,24 @@ static int cf_check map_range(
> unsigned long s, unsigned long e, void *data, unsigned long *c)
> {
> const struct map_data *map = data;
> +/* Start address of the BAR as seen by the guest. */
> +unsigned long start_gfn = PFN_DOWN(map->bar->guest_addr);
> +/* Physical start address of the BAR. */
> +mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr));
> int rc;
>
> for ( ; ; )
> {
> unsigned long size = e - s + 1;
> +/*
> + * Ranges to be mapped don't always start at the BAR start address,
> as
> + * there can be holes or partially consumed ranges. Account for the
> + * offset of the current address from the BAR start.
> + */
> +mfn_t map_mfn = mfn_add(start_mfn, s - start_gfn);
> +unsigned long m_end = mfn_x(map_mfn) + size - 1;
>
> -if ( !iomem_access_permitted(map->d, s, e) )
> +if ( !iomem_access_permitted(map->d, mfn_x(map_mfn), m_end) )
> {
> printk(XENLOG_G_WARNING
> "%pd denied access to MMIO range [%#lx, %#lx]\n",
> @@ -54,7 +66,8 @@ static int cf_check map_range(
> return -EPERM;
> }
>
> -rc = xsm_iomem_mapping(XSM_HOOK, map->d, s, e, map->map);
> +rc = xsm_iomem_mapping(XSM_HOOK, map->d, mfn_x(map_mfn), m_end,
> + map->map);
> if ( rc )
> {
> printk(XENLOG_G_WARNING
> @@ -72,8 +85,8 @@ static int cf_check map_range(
> * - {un}map_mmio_regions doesn't support preemption.
> */
>
> -rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
> - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
> +rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, map_mfn)
> + : unmap_mmio_regions(map->d, _gfn(s), size, map_mfn);
> if ( rc == 0 )
> {
> *c += size;
> @@ -82,8 +95,9 @@ static int cf_check map_range(
> if ( rc < 0 )
> {
> printk(XENLOG_G_WARNING
> - "Failed to identity %smap [%lx, %lx] for d%d: %d\n",
> - map->map ? "" : "un", s, e, map->d->domain_id, rc);
> + "Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
> + map->map ? "" : "un", s, e, mfn_x(map_mfn),
> + mfn_x(map_mfn) + size, map
xen | Successful pipeline for staging | 49818cde
Pipeline #1115598353 has passed! Project: xen ( https://gitlab.com/xen-project/xen ) Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging ) Commit: 49818cde ( https://gitlab.com/xen-project/xen/-/commit/49818cde637b5ec20383e46b71f93b2e7d867686 ) Commit Message: xen: remove asm/unaligned.h With include/xen/u... Commit Author: Juergen Gross ( https://gitlab.com/jgross1 ) Committed by: Andrew Cooper ( https://gitlab.com/andyhhp ) Pipeline #1115598353 ( https://gitlab.com/xen-project/xen/-/pipelines/1115598353 ) triggered by Ganis ( https://gitlab.com/ganis ) successfully completed 129 jobs in 3 stages. -- You're receiving this email because of your account on gitlab.com.
Re: hvmloader - allow_memory_relocate overlaps
On 2023-12-19 17:12, Jan Beulich wrote:
> On 16.12.2023 08:01, Neowutran wrote:
> > I am wondering if the variable "allow_memory_relocate" is still
> > relevant today and if its default value is still relevant.
> > Should it be defined to 0 by default instead of 1 (it seems to be a
> > workaround for qemu-traditional, so maybe an outdated default value ? ) ?
>
> So are you saying you use qemu-trad?
No, I am using "qemu_xen" ( from ��
xenstore-read -> 'device-model =
"qemu_xen"'
> Otherwise isn't libxl suppressing this behavior anyway?
If by "isn't libxl suppressing this behavior" you means if libxl is setting
the value of "allow_memory_relocate", then the answer is no.
Following this lead, I checked in what code path
"allow_memory_relocate" could be defined.
It is only defined in one code path,
In the file "tools/libs/light/libxl_dm.c",
in the function "void libxl__spawn_local_dm(libxl__egc *egc,
libxl__dm_spawn_state *dmss)":
'''
// ...
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
// ...
libxl__xs_printf(gc, XBT_NULL,
GCSPRINTF("%s/hvmloader/allow-memory-relocate", path),
"%d",
b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
!libxl__vnuma_configured(b_info));
// ...
'''
However, on QubesOS (my system), "local_dm" is never used, "stub_dm"
is always used.
In the function "void lib��
xl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)",
the value of "allow_memory_relocate" is never defined.
I tried to patch the code to define "allow_memory_relocate" in
"libxl__spawn_stub_dm":
'''
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2431,6 +2431,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc,
libxl__stub_dm_spawn_state *sdss)
libxl__xs_get_dompath(gc, guest_domid)),
"%s",
libxl_bios_type_to_string(guest_config->b_info.u.hvm.bios));
+libxl__xs_printf(gc, XBT_NULL,
+ libxl__sprintf(gc,
"%s/hvmloader/allow-memory-relocate", libxl__xs_get_dompath(gc, guest_domid)),
+ "%d",
+ 0);
}
ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
if (ret<0) {
'''
And it is indeed another way to solve my issue.
However I do not understand the xen hypervisor enough to known i��
f
"allow-memory-relocate" should have been defined in
"libxl__spawn_stub_dm" or if the real issue is somewhere else.
> Or did that code go stale?
>
> > Code extract:
> >
> > tools/firmware/hvmloader/pci.c
> > "
> >/*
> > * Do we allow hvmloader to relocate guest memory in order to
> > * increase the size of the lowmem MMIO hole? Defaulting to 1
> > * here will
> > mean that non-libxl toolstacks (including xend and
> > * home-grown ones) means that those using qemu-xen will still
> > * experience the memory relocation bug described below; but it
> > * also means that those using q
> > emu-traditional will *not*
> > * experience any change; and it also means that there is a
> > * work-around for those using qemu-xen, namely switching to
> > * qemu-traditional.
> > *
> > * If we defaulted to 0, and failing to resize the hole caused any
> > * problems with qemu-traditional, then there is no work-around.
> > *
> > * Since xend can��
only use qemu-traditional, I think this is the
> > * option that will have the least impact.
> > */
> > bool allow_memory_relocate = 1;
> > "
> >
> > "
> > /*
> > * At the moment qemu-xen can't deal with relocated memory regions.
> > * It's too close to the release to make a proper fix; for now,
> > * only allow t
> > he MMIO hole to grow large enough to move guest memory
> > * if we're running qemu-traditional. Items that don't fit will be
> > * relocated into the 64-bit address space.
> > *
> > * This loop now does the following:
> > * - If allow_memory_relocate, increase the MMIO hole until it's
> > * big enough, or
> > until it's 2GiB
> > * - If !allow_memory_relocate, increase the MMIO hole until it's
> > * big enough, or until it's 2GiB, or until it overlaps guest
> > * memory
> > */
> > while ( (mmio_total > (pci_mem_end - pci_mem_start))
> >��
&& ((pci_mem_start << 1) != 0)
> > && (allow_memory_relocate
> > || (((pci_mem_start << 1) >> PAGE_SHIFT)
> > >= hvm_info->low_mem_pgend)) )
> > pci_mem_start <<= 1;
> > "
> >
> > The issue it cause is documented in the source code: guest memory can
> > be trashed by the hvmloader.
> >
> > Due to this issue, it is impossible to passthrough a large PCI device
[libvirt test] 184199: tolerable all pass - PUSHED
flight 184199 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/184199/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184187 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184187 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184187 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-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-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: libvirt 9fc140c72d4a9d2ad44bd69474023e130b3da13b baseline version: libvirt 49f1406de814db6eec62d02096332059647ec5ad Last test of basis 184187 2023-12-20 04:19:12 Z1 days Testing same since 184199 2023-12-21 04:20:29 Z0 days1 attempts People who touched revisions under test: Egor Makrushin Göran Uddeborg Michal Privoznik 김인수 jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 pass test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw pass test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd 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 the
Re: [PATCH v2 37/39] xen/rirscv: add minimal amount of stubs to build full Xen
On Thu, 2023-12-21 at 09:02 +0100, Jan Beulich wrote: > On 20.12.2023 13:55, Oleksii wrote: > > On Mon, 2023-12-18 at 18:00 +0100, Jan Beulich wrote: > > > On 24.11.2023 11:30, Oleksii Kurochko wrote: > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/stubs.c > > > > @@ -0,0 +1,426 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > > > I think I can see why you need the former of these last two, but > > > do > > > you > > > really need the latter? > > It is needed for vm_event_request_t and vm_event_response_t, but if > > use > > a forward declaration that it won't be needed: > > > > typedef struct vm_event_st vm_event_request_t; > > typedef struct vm_event_st vm_event_response_t; > > Iirc Misra wouldn't like the duplicating of typedef-s used elsewhere. > But > as long as that's not going to stay (and I expect stubs.c to go away > before Misra becomes of concern for RISC-V), that's going to be okay, > I > think. Yet then avoiding the typedef-s and using struct vm_event_st > directly in the functions would be as good, and overall less code. Hmm, it makes sense to use sturct vm_event_st direcly in this case. Thanks for suggestion. ~ Oleksii
Re: [PATCH v6 3/9] xen/asm-generic: introduce generic div64.h header
Hi Oleksii, On 20/12/2023 14:08, Oleksii Kurochko wrote: All archs have the do_div implementation for BITS_PER_LONG == 64 so do_div64.h is moved to asm-generic. x86 and PPC were switched to asm-generic version of div64.h. Arm was switched partly because Arm has different implementation for 32-bits. Signed-off-by: Oleksii Kurochko Acked-by: Jan Beulich Acked-by: Shawn Anastasio Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v6 5/9] xen/asm-generic: introduce stub header numa.h
Hi Oleksii, On 20/12/2023 14:08, Oleksii Kurochko wrote: is common through some archs so it is moved to asm-generic. Signed-off-by: Oleksii Kurochko Reviewed-by: Michal Orzel Acked-by: Jan Beulich Acked-by: Shawn Anastasio I think this patch will need to be rebased on top of the lastest staging as this should clash with 51ffb3311895. Cheers, -- Julien Grall
Re: [PATCH v6 6/9] xen/asm-generic: introduce stub header softirq.h
Hi Oksii, On 20/12/2023 14:08, Oleksii Kurochko wrote: is common between Arm, PPC and RISC-V so it is moved to asm-generic. Drop Arm and PPC's softirq.h and use asm-generic version instead. Signed-off-by: Oleksii Kurochko Reviewed-by: Michal Orzel Acked-by: Jan Beulich Acked-by: Shawn Anastasio Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v6 7/9] xen: ifdef inclusion of in
Hi Oleksii, On 20/12/2023 14:08, Oleksii Kurochko wrote: Ifdef-ing inclusion of allows to avoid generation of empty for cases when CONFIG_GRANT_TABLE is not enabled. It would have been nice to explain the reason of this change. Is this a compilation error or just a nice thing to have? The reason I am asking is... The following changes were done for Arm: should be included directly because it contains gnttab_dom0_frames() macros which is unique for Arm and is used in arch/arm/domain_build.c. is #ifdef-ed with CONFIG_GRANT_TABLE in so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames won't be available for use in arch/arm/domain_build.c. ... I find rather ugly that we require domain_build.c to include both asm/grant_table.h and xen/grant_table.h. Right now, I don't have a better approach, so I would be ok so long the rationale of the change is explained in the commit message. Suggested-by: Jan Beulich Signed-off-by: Oleksii Kurochko Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v6 7/9] xen: ifdef inclusion of in
On 21/12/2023 19:19, Julien Grall wrote: Hi Oleksii, On 20/12/2023 14:08, Oleksii Kurochko wrote: Ifdef-ing inclusion of allows to avoid generation of empty for cases when CONFIG_GRANT_TABLE is not enabled. It would have been nice to explain the reason of this change. Is this a compilation error or just a nice thing to have? The reason I am asking is... The following changes were done for Arm: should be included directly because it contains gnttab_dom0_frames() macros which is unique for Arm and is used in arch/arm/domain_build.c. is #ifdef-ed with CONFIG_GRANT_TABLE in so in case of !CONFIG_GRANT_TABLE gnttab_dom0_frames won't be available for use in arch/arm/domain_build.c. ... I find rather ugly that we require domain_build.c to include both asm/grant_table.h and xen/grant_table.h. Right now, I don't have a better approach, so I would be ok so long the rationale of the change is explained in the commit message. Urgh, I just realized that this is explained in the commit message. Please ignore my comment about expanding the commit message. Sorry for the noise. Cheers, -- Julien Grall
Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h
Hi,
On 20/12/2023 14:08, Oleksii Kurochko wrote:
Arm, PPC and RISC-V use the same device.h thereby device.h
was moved to asm-generic. Arm's device.h was taken as a base with
the following changes:
- #ifdef PCI related things.
- #ifdef ACPI related things.
- Rename #ifdef guards.
- Add SPDX tag.
- #ifdef CONFIG_HAS_DEVICE_TREE related things.
- #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.
Also Arm and PPC are switched to asm-generic version of device.h
Signed-off-by: Oleksii Kurochko
---
Jan wrote the following:
Overall I think there are too many changes done all in one go here.
But it's mostly Arm which is affected, so I'll leave judging about that
to the Arm maintainers.
Arm maintainers, will it be fine for you to not split the patch?
So in general I agree with Jan, patches should be kept small so they are
easy to review.
Given the discussion has been on-going for a while (we are at v6), I
will give an attempt to review the patch as-is. But in the future,
please try to split. The smaller it is, the easier to review. For code
movement and refactoring, I tend to first have a few refactoring patches
and then move the code in a separate patch. So it is easier to spot the
differences.
---
Changes in V6:
- Rebase only.
---
Changes in V5:
- Removed generated file: xen/include/headers++.chk.new
- Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
- Updated the commit message
- Switched Arm and PPC to asm-generic version of device.h
- Replaced HAS_PCI with CONFIG_HAS_PCI
- ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
- ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
- ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
- Updated the commit message ( remove a note with question about
if device.h should be in asm-generic or not )
- Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
- Rationalized usage of CONFIG_HAS_* in device.h
- Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
Changes in V3:
- ifdef device tree related things.
- update the commit message
---
Changes in V2:
- take ( as common ) device.h from Arm as PPC and RISC-V use it as a
base.
- #ifdef PCI related things.
- #ifdef ACPI related things.
- rename DEVICE_GIC to DEVIC_IC.
- rename #ifdef guards.
- switch Arm and PPC to generic device.h
- add SPDX tag
- update the commit message
---
xen/arch/arm/device.c | 15 ++-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/gic-v2.c | 4 +-
xen/arch/arm/gic-v3.c | 6 +-
xen/arch/arm/gic.c| 4 +-
xen/arch/arm/include/asm/Makefile | 1 +
xen/arch/ppc/include/asm/Makefile | 1 +
xen/arch/ppc/include/asm/device.h | 53
.../asm => include/asm-generic}/device.h | 125 +++---
9 files changed, 102 insertions(+), 109 deletions(-)
delete mode 100644 xen/arch/ppc/include/asm/device.h
rename xen/{arch/arm/include/asm => include/asm-generic}/device.h (79%)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 1f631d3274..affbe79f9a 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -16,7 +16,10 @@
#include
extern const struct device_desc _sdevice[], _edevice[];
+
+#ifdef CONFIG_ACPI
extern const struct acpi_device_desc _asdevice[], _aedevice[];
+#endif
int __init device_init(struct dt_device_node *dev, enum device_class class,
const void *data)
@@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node *dev, enum
device_class class,
return -EBADF;
}
+#ifdef CONFIG_ACPI
int __init acpi_device_init(enum device_class class, const void *data, int
class_type)
{
const struct acpi_device_desc *desc;
@@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class class, const
void *data, int class
return -EBADF;
}
+#endif
enum device_class device_get_class(const struct dt_device_node *dev)
{
@@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct dt_device_node
*dev, p2m_type_t p2mt,
struct map_range_data mr_data = {
.d = d,
.p2mt = p2mt,
-.skip_mapping = !own_device ||
-(is_pci_passthrough_enabled() &&
-(device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+.skip_mapping =
+!own_device
+#ifdef CONFIG_HAS_PCI
+|| (is_pci_passthrough_enabled() &&
+(device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))
+#endif
So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not defined.
It is not clear what's
[xen-unstable-smoke test] 184205: tolerable all pass - PUSHED
flight 184205 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/184205/ 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 49818cde637b5ec20383e46b71f93b2e7d867686 baseline version: xen 3909fb4692dfbf7e46c0bcc37b0a3b943a034da9 Last test of basis 184203 2023-12-21 11:00:27 Z0 days Testing same since 184205 2023-12-21 15:00:26 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Arnd Bergmann Jan Beulich Juergen Gross Roger Pau Monne Roger Pau Monné 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 3909fb4692..49818cde63 49818cde637b5ec20383e46b71f93b2e7d867686 -> smoke
Re: [PATCH v2 30/39] xen/riscv: define an address of frame table
On Mon, 2023-12-18 at 12:22 +0100, Jan Beulich wrote:
> On 18.12.2023 11:36, Oleksii wrote:
> > On Thu, 2023-12-14 at 16:48 +0100, Jan Beulich wrote:
> > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > +#define SLOTN_ENTRY_SIZE SLOTN(1)
> > > > +
> > > > #define XEN_VIRT_START 0xC000 /* (_AC(-1, UL) + 1
> > > > -
> > > > GB(1)) */
> > > > +
> > > > +#define FRAMETABLE_VIRT_START SLOTN(196)
> > > > +#define FRAMETABLE_SIZE GB(3)
> > > > +#define FRAMETABLE_NR (FRAMETABLE_SIZE /
> > > > sizeof(*frame_table))
> > > > +#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START +
> > > > FRAMETABLE_SIZE - 1)
> > > > +
> > > > +#define VMAP_VIRT_START SLOTN(194)
> > > > +#define VMAP_VIRT_SIZE GB(1)
> > >
> > > May I suggest that you keep these blocks sorted by slot number?
> > > Or
> > > wait,
> > > the layout comment further up is also in decreasing order, so
> > > that's
> > > fine here, but then can all of this please be moved next to the
> > > comment
> > > actually providing the necessary context (thus eliminating the
> > > need
> > > for
> > > new comments)?
> > Sure, I'll put this part close to layout comment.
> >
> > > You'll then also notice that the generalization here
> > > (keeping basically the same layout for e.g. SATP_MODE_SV48, just
> > > shifted
> > > by 9 bits) isn't in line with the comment there.
> > Does it make sense to add another one table with updated addresses
> > for
> > SATP_MODE_SV48?
>
> Well, especially if you mean to support that mode, its layout surely
> wants writing down. I was hoping though that maybe you/we could get
> away
> without multiple tables, but e.g. use one having multiple columns.
I came up with the following but I am not sure that it is really
convient:
/*
* RISC-V64 Layout:
*
#if RV_STAGE1_MODE == SATP_MODE_SV39
*
* From the riscv-privileged doc:
* When mapping between narrower and wider addresses,
* RISC-V zero-extends a narrower physical address to a wider size.
* The mapping between 64-bit virtual addresses and the 39-bit usable
* address space of Sv39 is not based on zero-extension but instead
* follows an entrenched convention that allows an OS to use one or
* a few of the most-significant bits of a full-size (64-bit) virtual
* address to quickly distinguish user and supervisor address
regions.
*
* It means that:
* top VA bits are simply ignored for the purpose of translating to
PA.
#endif
*
* SATP_MODE_SV32 SATP_MODE_SV39 SATP_MODE_SV48
SATP_MODE_SV57
*
---
* BA0 | FFE0 | C000 | FF80 |
* BA1 | 1900 | 0032 | 6400 |
00C8
* BA2 | 1880 | 0031 | 6200 |
00C4
* BA3 | 1840 | 00308000 | 6100 |
00C2
*
*
===
=
* Start addr| End addr | Size | Slot |area
description
*
===
=
* BA0 + 0x80 | |1016 MB |
L${HYP_PT_ROOT_LEVEL} 511 | Unused
* BA0 + 0x40 | BA0 + 0x80 | 2 MB |
L${HYP_PT_ROOT_LEVEL} 511 | Fixmap
* BA0 + 0x20 | BA0 + 0x40 | 4 MB |
L${HYP_PT_ROOT_LEVEL} 511 | FDT
* BA0| BA0 + 0x20 | 2 MB |
L${HYP_PT_ROOT_LEVEL} 511 | Xen
* ... | 1 GB |
L${HYP_PT_ROOT_LEVEL} 510 | Unused
* BA1 + 0x00 | BA1 + 0x4D8000 | 309 GB |
L${HYP_PT_ROOT_LEVEL} 200-509 | Direct map
* ... | 1 GB |
L${HYP_PT_ROOT_LEVEL} 199 | Unused
* BA2 + 0x00 | BA2 + 0xC000 | 3 GB |
L${HYP_PT_ROOT_LEVEL} 196-198 | Frametable
* ... | 1 GB |
L${HYP_PT_ROOT_LEVEL} 195 | Unused
* BA3 + 0x00 | BA3 + 0x4000 | 1 GB |
L${HYP_PT_ROOT_LEVEL} 194 | VMAP
* ... | 194 GB |
L${HYP_PT_ROOT_LEVEL} 0 - 193 | Unused
*
===
=
*/
Do you have better ideas?
Thanks in advamce.
~ Oleksii
>
>
> > Microchip has h/w which requires SATP_MODE_SV48 ( at least ), so I
> > have
> > a patch which introduces SATP_MODE_SV48 and I planned to update the
> > layout table in this patch.
>
>
>
Re: [BUG]i2c_hid_acpi broken with 4.17.2 on Framework Laptop 13 AMD
Le jeu. 21 déc. 2023 à 14:29, Juergen Gross a écrit :
> On 21.12.23 13:40, Jan Beulich wrote:
> > On 20.12.2023 17:34, Sébastien Chaumat wrote:
> >> Here are the patches I made to xen and linux kernel
> >> Plus dmesg (bare metal,xen) and "xl dmesg"
> >
> > So the problem looks to be that pci_xen_initial_domain() results in
> > permanent setup of IRQ7, when there only "static" ACPI tables (in
> > particular source overrides in MADT) are consulted. The necessary
> > settings, however, are known only after _CRS for the device was
> > evaluated (and possibly _PRS followed by invocation of _SRS). All of
> > this happens before xen_register_gsi() is called. But that function's
> > call to xen_register_pirq() is short-circuited by the very first if()
> > in xen_register_pirq() when there was an earlier invocation. Hence
> > the (wrong) "edge" binding remains in place, as was established by
> > the earlier call here.
> >
> > Jürgen, there's an interesting comment in xen_bind_pirq_gsi_to_irq(),
> > right before invoking irq_set_chip_and_handler_name(). Despite what
> > the comment says (according to my reading), the fasteoi one is _not_
> > used in all cases. Assuming there's a reason for this, it's not clear
> > to me whether updating the handler later on is a valid thing to do.
> > __irq_set_handler() being even an exported symbol suggests that might
> > be an option to use here. Then again merely updating the handler may
> > not be sufficient, seeing there are also e.g. IRQD_TRIGGER_MASK and
> > IRQD_LEVEL.
>
> I understand the last paragraph of that comment to reason, that in case
> pirq_needs_eoi() return true even in case of an edge triggered interrupt,
> the outcome is still okay.
>
> I don't think updating the handler later is valid.
>
> > Sébastien, to prove the (still pretty weak) theory that the change in
> > handler is all that's needed to make things work in your case, could
> > you fiddle with pci_xen_initial_domain() to have it skip IRQ7? (That
> > of course won't be a proper solution, but ought to be okay for your
> > system.) The main weakness of the theory is that IRQ7 really isn't
> > very special in this regard - other PCI IRQs routed to the low 16
> > IO-APIC pins ought to have similar issues (from the log, on your
> > system this would be at least IRQ6 and IRQ10, except that they happen
> > to be edge/low, so fitting the ISA defaults); only IRQ16 and up would
> > work okay.
> >
>
Doing just that : IQR7 is now of type level
xen-pirq -ioapic-level pinctrl_amd
(but is ioapic-level there totally equivalent to the fasteoi of baremetal)
Still the touchpad does not work.
And we have now :
Dec 21 20:13:57 fedora kernel: i2c_hid_acpi i2c-PIXA3854:00: failed to
reset device: -61
Dec 21 20:14:17 fedora kernel: i2c_hid_acpi: probe of i2c-PIXA3854:00
failed with error -61
in addition to
Dec 21 20:13:57 fedora kernel: i2c_hid_acpi i2c-FRMW0004:00: failed to
reset device: -61
Dec 21 20:13:57 fedora kernel: i2c_hid_acpi i2c-FRMW0005:00: failed to
reset device: -61
Dec 21 20:14:17 fedora kernel: i2c_hid_acpi: probe of i2c-FRMW0004:00
failed with error -61
Dec 21 20:14:17 fedora kernel: i2c_hid_acpi: probe of i2c-FRMW0005:00
failed with error -61
I noticed that on baremetal :
53: 0 0 0 0 0 1268
0 0 0 0 0 0 0
0 0 0 amd_gpio5 FRMW0005:00
54: 0 0 0 0 0 1
0 0 0 0 0 0 0
0 0 0 amd_gpio 84 FRMW0004:00
55: 0 0 0 0 0 1403
0 0 0 0 0 0 0
0 0 0 amd_gpio8 PIXA3854:00
with xen with IRQ7 setup only once there's only (the touchpad is
PIXA3854:00)
176: 0 0 0 0 0 0
1 0 0 0 0 0 0
0 0 0 amd_gpio8
Interestingly when IRQ7 is setup twice (normal xen)
176: 0 0 0 0 0 0
1 0 0 0 0 0 0
0 0 0 amd_gpio8 PIXA3854:00
> Furthermore it might be interesting to know whether ELCR would give us
> > any hint in this case. Sadly depending on where you look,
> > applicability of this pair of registers (I/O ports 0x4d0 and 0x4d1)
> > to other than EISA systems is claimed true or false. Could you perhaps
> > make Xen simply log the values read from those two ports, by e.g.
> > inserting
> >
> > printk("ELCR: %02x, %02x\n", inb(0x4d0), inb(0x4d1));
> >
> > in, say, setup_dump_irqs()?
> >
>
did that but I don't know how to trigger the dump.
Sébastien
[linux-linus test] 184201: tolerable FAIL - PUSHED
flight 184201 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/184201/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184194 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184194 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184194 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184194 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184194 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184194 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184194 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184194 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-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-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-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 test-armhf-armhf-xl-rtds 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-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-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: linuxa4aebe936554dac6a91e5d091179c934f8325708 baseline version: linux1a44b0073b9235521280e19d963b6dfef7888f18 Last test of basis 184194 2023-12-20 20:40:45 Z1 days Testing same since 184201 2023-12-21 08:46:02 Z0 days1 attempts People who touched revisions under test: Alexander Gordeev Andrew Davis Arnd Bergmann Chukun Pan Geert Uytterhoeven Heiko Carstens Ilpo Järvinen Jernej Skrabec Krzysztof Kozlowski Kunwu Chan Linus Torvalds Macpaul Lin Mario Limonciello Paulo Alcantara (SUSE) Paulo Alcantara Philipp Zabel Rajvi Jingar Sami Tolvanen Shyam Prasad N Steve French Tony Lindgren Vineeth Vijayan Vishnu Sankar Zizhi Wo jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64
Re: [PATCH v10 12/17] vpci/header: emulate PCI_COMMAND register for guests
On 11/21/23 09:17, Roger Pau Monné wrote: > On Thu, Oct 12, 2023 at 10:09:18PM +, Volodymyr Babchuk wrote: >> From: Oleksandr Andrushchenko >> >> Xen and/or Dom0 may have put values in PCI_COMMAND which they expect >> to remain unaltered. PCI_COMMAND_SERR bit is a good example: while the >> guest's view of this will want to be zero initially, the host having set >> it to 1 may not easily be overwritten with 0, or else we'd effectively >> imply giving the guest control of the bit. Thus, PCI_COMMAND register needs >> proper emulation in order to honor host's settings. >> >> According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2 >> Device Control" the reset state of the command register is typically 0, >> so when assigning a PCI device use 0 as the initial state for the guest's >> view >> of the command register. >> >> Here is the full list of command register bits with notes about >> emulation, along with QEMU behavior in the same situation: >> >> PCI_COMMAND_IO - QEMU does not allow a guest to change value of this bit >> in real device. Instead it is always set to 1. As far as I can tell QEMU only sets this bit to 1 (in hardware) if it exposes an I/O BAR to the guest. >> A guest can write to this >> register, but writes are ignored. In Xen, I think we should use rsvdp_mask for domUs for now since we don't (yet) support I/O BARs for domUs in vPCI. >> >> PCI_COMMAND_MEMORY - QEMU behaves exactly as with PCI_COMMAND_IO. In >> Xen case, we handle writes to this bit by mapping/unmapping BAR >> regions. For devices assigned to DomUs, memory decoding will be >> disabled and the initialization. >> >> PCI_COMMAND_MASTER - Allow guest to control it. QEMU passes through >> writes to this bit. >> >> PCI_COMMAND_SPECIAL - Guest can generate special cycles only if it has >> access to host bridge that supports software generation of special >> cycles. In our case guest has no access to host bridges at all. Value >> after reset is 0. QEMU passes through writes of this bit, we will do >> the same. >> >> PCI_COMMAND_INVALIDATE - Allows "Memory Write and Invalidate" commands >> to be generated. It requires additional configuration via Cacheline >> Size register. We are not emulating this register right now and we >> can't expect guest to properly configure it. QEMU "emulates" access to >> Cachline Size register by ignoring all writes to it. QEMU passes through >> writes of PCI_COMMAND_INVALIDATE bit, we will do the same. >> >> PCI_COMMAND_VGA_PALETTE - Enable VGA palette snooping. QEMU passes >> through writes of this bit, we will do the same. >> >> PCI_COMMAND_PARITY - Controls how device response to parity >> errors. QEMU ignores writes to this bit, we will do the same. >> >> PCI_COMMAND_WAIT - Reserved. Should be 0, but QEMU passes >> through writes of this bit, so we will do the same. Actually, PCI_COMMAND_WAIT bit is in qemu's res_mask, meaning it only passes through the writes in permissive mode. By default qemu does not pass through writes. PCI LB 3.0 and PCIe 6.1 specifications say devices should hardwire this bit to 0, but that some devices may have implemented it as RW. So I think we should use rsvdp_mask in Xen for this bit. >> >> PCI_COMMAND_SERR - Controls if device can assert SERR. QEMU ignores >> writes to this bit, we will do the same. >> >> PCI_COMMAND_FAST_BACK - Optional bit that allows fast back-to-back >> transactions. It is configured by firmware, so we don't want guest to >> control it. QEMU ignores writes to this bit, we will do the same. >> >> PCI_COMMAND_INTX_DISABLE - Disables INTx signals. If MSI(X) is >> enabled, device is prohibited from asserting INTx as per >> specification. Value after reset is 0. In QEMU case, it checks of INTx >> was mapped for a device. If it is not, then guest can't control >> PCI_COMMAND_INTX_DISABLE bit. In our case, we prohibit a guest to >> change value of this bit if MSI(X) is enabled. > > FWIW, bits 11-15 are RsvdP, so we will need to add support for them > after the series from Stewart that adds support for register masks is > accepted. I am working on this. > >> >> Signed-off-by: Oleksandr Andrushchenko >> Signed-off-by: Volodymyr Babchuk >> --- >> In v10: >> - Added cf_check attribute to guest_cmd_read >> - Removed warning about non-zero cmd >> - Updated comment MSI code regarding disabling INTX >> - Used ternary operator in vpci_add_register() call >> - Disable memory decoding for DomUs in init_bars() >> In v9: >> - Reworked guest_cmd_read >> - Added handling for more bits >> Since v6: >> - fold guest's logic into cmd_write >> - implement cmd_read, so we can report emulated INTx state to guests >> - introduce header->guest_cmd to hold the emulated state of the >> PCI_COMMAND register for guests >> Since v5: >> - add additional check for MSI-X enabled while altering INTX bit >> - make sure INTx disabled while guests enable MSI/MSI-X >> Since v3: >> - gate more code on CONFIG_HAS_MSI >> - removed logic for the case when MSI/M
Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
On Thu, 21 Dec 2023, Andrew Cooper wrote: > On 21/12/2023 10:29 am, Federico Serafini wrote: > > On 20/12/23 22:23, Andrew Cooper wrote: > >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote: > >>> On Wed, 20 Dec 2023, Federico Serafini wrote: > On 20/12/23 12:55, Jan Beulich wrote: > > On 20.12.2023 12:48, Julien Grall wrote: > >> On 20/12/2023 11:03, Federico Serafini wrote: > >>> --- a/xen/arch/arm/arm64/vsysreg.c > >>> +++ b/xen/arch/arm/arm64/vsysreg.c > >>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > >>> /* RO at EL0. RAZ/WI at EL1 */ > >>> if ( regs_mode_is_user(regs) ) > >>> return handle_ro_raz(regs, regidx, > >>> hsr.sysreg.read, hsr, > >>> 0); > >>> - else > >>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>> hsr, > >>> 1); > >>> + > >>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>> hsr, 1); > >> I don't 100% like this change (mostly because I find if/else clearer > >> here). > > While (it doesn't matter here) my view on this is different, I'm > > still > > puzzled why the tool would complain / why a change here is necessary. > > It is not _one_ return statement, but there's still (and > > obviously) no > > way of falling through. > The tool is configurable: > if you prefer deviate these cases instead of refactoring the code > I can update the configuration. > >>> If you say "deviation", it implies that there is a violation. I think > >>> Jan's point was that both code versions shouldn't be any different. > >>> > >>> # option-1 > >>> if (a) > >>> return f1(); > >>> else > >>> return f2(); > >>> > >>> # option-2 > >>> if (a) > >>> return f1(); > >>> return f2(); > >>> > >>> Both options are equally guaranteed to always return. It looks like a > >>> peculiar limitation to only recognize option-2 as something that > >>> returns > >>> 100% of the times. Also option-1 returns 100% of the times. What am I > >>> missing? > > > > I don't think this is necessarily a limitation because it highlights > > cases where an if-else could be replaced with a simple if: > > some may find an if-else clearer, other may find the single if clearer. > > > > From a safety point of view both options are safe because there > > is no risk of unintentional fall through. > > > > If you all agree on the fact that small code refactoring like the ones I > > proposed are counterproductive, then I can update the tool configuration > > to consider also option-1 as safe. > > I would certainly prefer if we could continue to use option 1. Yes, that is my preference too
Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3
On Thu, 21 Dec 2023, Jan Beulich wrote: > On 20.12.2023 13:15, Federico Serafini wrote: > > On 20/12/23 12:55, Jan Beulich wrote: > >> On 20.12.2023 12:48, Julien Grall wrote: > >>> On 20/12/2023 11:03, Federico Serafini wrote: > --- a/xen/arch/arm/arm64/vsysreg.c > +++ b/xen/arch/arm/arm64/vsysreg.c > @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > /* RO at EL0. RAZ/WI at EL1 */ > if ( regs_mode_is_user(regs) ) > return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, > 0); > -else > -return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > + > +return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > >>> > >>> I don't 100% like this change (mostly because I find if/else clearer > >>> here). > >> > >> While (it doesn't matter here) my view on this is different, I'm still > >> puzzled why the tool would complain / why a change here is necessary. > >> It is not _one_ return statement, but there's still (and obviously) no > >> way of falling through. > > > > The tool is configurable: > > if you prefer deviate these cases instead of refactoring the code > > I can update the configuration. > > I guess this then needs to be discussed on the first call in the new year. > Stefano - can you take note of that, please? Yes, will do
[xen-unstable test] 184204: tolerable FAIL - PUSHED
flight 184204 xen-unstable real [real] flight 184207 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/184204/ http://logs.test-lab.xenproject.org/osstest/logs/184207/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 184207-retest test-armhf-armhf-xl-vhd 13 guest-start fail pass in 184207-retest Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-vhd 14 migrate-support-check fail in 184207 never pass test-armhf-armhf-xl-vhd 15 saverestore-support-check fail in 184207 never pass test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184197 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184197 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184197 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184197 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184197 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184197 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184197 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184197 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184197 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184197 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184197 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184197 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail 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-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 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-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-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-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail 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-amd64-amd64-libvirt-vhd 14 migrate-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-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-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 3909fb4692dfbf7e46c0bcc37b0a3b943a034da9 baseline version: xen e
[GIT PULL] xen: branch for v6.7-rc7
Linus, Please git pull the following tag: git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-6.7a-rc7-tag xen: branch for v6.7-rc7 It contains a single patch fixing a build issue for x86 32-bit configurations with CONFIG_XEN, which was introduced in the 6.7 development cycle. Thanks. Juergen arch/x86/xen/Kconfig | 1 + 1 file changed, 1 insertion(+) Arnd Bergmann (1): x86/xen: add CPU dependencies for 32-bit build
